std::pair not trivially copyable?

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

std::pair not trivially copyable?

Hubert Tong via cfe-dev
We had someone trip over a case that reduces to this:

F:\scratch>type tc6347.cpp
#include <utility>
std::pair<unsigned, bool> data[10];
void bar() {
  for (const auto item : data)
    (void)item.first;
}

F:\scratch>%wo%\clang -c -Wall tc6347.cpp
tc6347.cpp:4:19: warning: loop variable 'item' of type 'const std::pair<unsigned
      int, bool>' creates a copy from type 'const std::pair<unsigned int, bool>'
      [-Wrange-loop-construct]
  for (const auto item : data)
                  ^
tc6347.cpp:4:8: note: use reference type 'const std::pair<unsigned int, bool> &'
      to prevent copying
  for (const auto item : data)
       ^~~~~~~~~~~~~~~~~
                  &
1 warning generated.

F:\scratch>


This used to compile cleanly.  It turns out that the diagnostic
used to be suppressed for POD types, and now it is suppressed for
trivially-copyable types.

So... a std::pair of two scalars is not trivially copyable?
I'd have thought it would be, but then C++ is full of surprises.

Thanks,
--paulr

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: std::pair not trivially copyable?

Hubert Tong via cfe-dev


On Tue, Aug 25, 2020 at 5:52 AM Robinson, Paul via cfe-dev <[hidden email]> wrote:
We had someone trip over a case that reduces to this:

F:\scratch>type tc6347.cpp
#include <utility>
std::pair<unsigned, bool> data[10];
void bar() {
  for (const auto item : data)
    (void)item.first;
}

F:\scratch>%wo%\clang -c -Wall tc6347.cpp
tc6347.cpp:4:19: warning: loop variable 'item' of type 'const std::pair<unsigned
      int, bool>' creates a copy from type 'const std::pair<unsigned int, bool>'
      [-Wrange-loop-construct]
  for (const auto item : data)
                  ^
tc6347.cpp:4:8: note: use reference type 'const std::pair<unsigned int, bool> &'
      to prevent copying
  for (const auto item : data)
       ^~~~~~~~~~~~~~~~~
                  &
1 warning generated.

F:\scratch>


This used to compile cleanly.  It turns out that the diagnostic
used to be suppressed for POD types, and now it is suppressed for
trivially-copyable types.

So... a std::pair of two scalars is not trivially copyable?

Looks like it isn't trivially copy /assignable/, but it is trivially copy constructible in both libc++ and libstdc++: https://godbolt.org/z/Md9f78

So, one question (why is std::pair of two trivial types not trivially assignable?) and /probably/ one bug (why does the warning flag on a trivially copy constructible type).

From what I can tell, reading this ( https://stackoverflow.com/questions/58283694/why-is-pair-of-const-trivially-copyable-but-pair-is-not ) and the C++17 spec, it just doesn't specify the copy and move assignment operators as defaulted, or say anything about their triviality, so they aren't trivial even for trivial types. Perhaps an oversight when thinking about the other complexities of when they shuold be deleted. 

I'd have thought it would be, but then C++ is full of surprises.

Thanks,
--paulr

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: std::pair not trivially copyable?

Hubert Tong via cfe-dev
On Tue, Aug 25, 2020 at 4:37 PM David Blaikie via cfe-dev <[hidden email]> wrote:

From what I can tell, reading this ( https://stackoverflow.com/questions/58283694/why-is-pair-of-const-trivially-copyable-but-pair-is-not ) and the C++17 spec, it just doesn't specify the copy and move assignment operators as defaulted, or say anything about their triviality, so they aren't trivial even for trivial types. Perhaps an oversight when thinking about the other complexities of when they shuold be deleted. 

In general, move/copy assignment cannot be defaulted for pair, because assignment of reference types has meaning:

int i = 2;
int j = 3;
std::pair<int&, int> p1(i, 5); 
std::pair<int&, int> p2(j, 7);
p2 = p1;  // j now has the value 2


struct A
{
    A(int& r_, int i_) : r(r_), i(i_) {}
    int& r;
    int i;
};

int i = 2;
int j = 3;
A a1(i, 5);
A a2(j, 7);
a2 = a1;  // Compile time error - deleted copy assignment operator


Now,  this doesn't that pair couldn't have trivial copy/move assignment operators when it holds trivially copy/move assignable types, but I don't know how much of an ABI break this would be.
--
 Nevin ":-)" Liber  <mailto:[hidden email]>  +1-847-691-1404

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: std::pair not trivially copyable?

Hubert Tong via cfe-dev
On Wed, 26 Aug 2020 at 10:18, Nevin Liber via cfe-dev <[hidden email]> wrote:
On Tue, Aug 25, 2020 at 4:37 PM David Blaikie via cfe-dev <[hidden email]> wrote:

From what I can tell, reading this ( https://stackoverflow.com/questions/58283694/why-is-pair-of-const-trivially-copyable-but-pair-is-not ) and the C++17 spec, it just doesn't specify the copy and move assignment operators as defaulted, or say anything about their triviality, so they aren't trivial even for trivial types. Perhaps an oversight when thinking about the other complexities of when they shuold be deleted. 

In general, move/copy assignment cannot be defaulted for pair, because assignment of reference types has meaning:

int i = 2;
int j = 3;
std::pair<int&, int> p1(i, 5); 
std::pair<int&, int> p2(j, 7);
p2 = p1;  // j now has the value 2


struct A
{
    A(int& r_, int i_) : r(r_), i(i_) {}
    int& r;
    int i;
};

int i = 2;
int j = 3;
A a1(i, 5);
A a2(j, 7);
a2 = a1;  // Compile time error - deleted copy assignment operator


Now,  this doesn't that pair couldn't have trivial copy/move assignment operators when it holds trivially copy/move assignable types, but I don't know how much of an ABI break this would be.

Making std::pair be trivially-copyable whenever possible would affect whether std::pair is POD for the purpose of layout, which could result in an ABI break for at least any code that derives from std::pair or that contains a [[no_unique_address]] std::pair member: https://godbolt.org/z/GTvo4r

This is certainly not something we could do for the libc++ stable ABI. Maybe for the unstable ABI, but given the above change only makes layout worse, it's not clear that there would be a strong motivation. We already give std::pair trivial copy/move construction and trivial destruction whenever possible, so we can pass and return it efficiently.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: std::pair not trivially copyable?

Hubert Tong via cfe-dev


On Wed, Aug 26, 2020 at 11:34 AM Richard Smith via cfe-dev <[hidden email]> wrote:
On Wed, 26 Aug 2020 at 10:18, Nevin Liber via cfe-dev <[hidden email]> wrote:
On Tue, Aug 25, 2020 at 4:37 PM David Blaikie via cfe-dev <[hidden email]> wrote:

From what I can tell, reading this ( https://stackoverflow.com/questions/58283694/why-is-pair-of-const-trivially-copyable-but-pair-is-not ) and the C++17 spec, it just doesn't specify the copy and move assignment operators as defaulted, or say anything about their triviality, so they aren't trivial even for trivial types. Perhaps an oversight when thinking about the other complexities of when they shuold be deleted. 

In general, move/copy assignment cannot be defaulted for pair, because assignment of reference types has meaning:

int i = 2;
int j = 3;
std::pair<int&, int> p1(i, 5); 
std::pair<int&, int> p2(j, 7);
p2 = p1;  // j now has the value 2


struct A
{
    A(int& r_, int i_) : r(r_), i(i_) {}
    int& r;
    int i;
};

int i = 2;
int j = 3;
A a1(i, 5);
A a2(j, 7);
a2 = a1;  // Compile time error - deleted copy assignment operator


Now,  this doesn't that pair couldn't have trivial copy/move assignment operators when it holds trivially copy/move assignable types, but I don't know how much of an ABI break this would be.

Making std::pair be trivially-copyable whenever possible would affect whether std::pair is POD for the purpose of layout, which could result in an ABI break for at least any code that derives from std::pair or that contains a [[no_unique_address]] std::pair member: https://godbolt.org/z/GTvo4r

This is certainly not something we could do for the libc++ stable ABI. Maybe for the unstable ABI, but given the above change only makes layout worse, it's not clear that there would be a strong motivation. We already give std::pair trivial copy/move construction and trivial destruction whenever possible, so we can pass and return it efficiently.

Interesting - so POD for the purpose of layout is... not good? or more restrictive on the layout than if not. (probably an impractical idea, but would it be reasonable then to have an attribute that says "not POD for the purposes of layout" but I guess that'd have to be cross-compiler/etc/etc... probably too much work)

What's the cost of pair having non-trivial copy assignment? More expensive vector of pair resizing (though I guess the copy ctor calls would be readily optimized away & LLVM could get that back to a memcpy anyway?)?

(the original issue with the warning could be fixed by narrowing the warning to only worry about trivial copy construction, not trivial copyability in general)

- Dave 

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: std::pair not trivially copyable?

Hubert Tong via cfe-dev
On Wed, 26 Aug 2020 at 11:42, David Blaikie <[hidden email]> wrote:
On Wed, Aug 26, 2020 at 11:34 AM Richard Smith via cfe-dev <[hidden email]> wrote:
On Wed, 26 Aug 2020 at 10:18, Nevin Liber via cfe-dev <[hidden email]> wrote:
On Tue, Aug 25, 2020 at 4:37 PM David Blaikie via cfe-dev <[hidden email]> wrote:

From what I can tell, reading this ( https://stackoverflow.com/questions/58283694/why-is-pair-of-const-trivially-copyable-but-pair-is-not ) and the C++17 spec, it just doesn't specify the copy and move assignment operators as defaulted, or say anything about their triviality, so they aren't trivial even for trivial types. Perhaps an oversight when thinking about the other complexities of when they shuold be deleted. 

In general, move/copy assignment cannot be defaulted for pair, because assignment of reference types has meaning:

int i = 2;
int j = 3;
std::pair<int&, int> p1(i, 5); 
std::pair<int&, int> p2(j, 7);
p2 = p1;  // j now has the value 2


struct A
{
    A(int& r_, int i_) : r(r_), i(i_) {}
    int& r;
    int i;
};

int i = 2;
int j = 3;
A a1(i, 5);
A a2(j, 7);
a2 = a1;  // Compile time error - deleted copy assignment operator


Now,  this doesn't that pair couldn't have trivial copy/move assignment operators when it holds trivially copy/move assignable types, but I don't know how much of an ABI break this would be.

Making std::pair be trivially-copyable whenever possible would affect whether std::pair is POD for the purpose of layout, which could result in an ABI break for at least any code that derives from std::pair or that contains a [[no_unique_address]] std::pair member: https://godbolt.org/z/GTvo4r

This is certainly not something we could do for the libc++ stable ABI. Maybe for the unstable ABI, but given the above change only makes layout worse, it's not clear that there would be a strong motivation. We already give std::pair trivial copy/move construction and trivial destruction whenever possible, so we can pass and return it efficiently.

Interesting - so POD for the purpose of layout is... not good? or more restrictive on the layout than if not. (probably an impractical idea, but would it be reasonable then to have an attribute that says "not POD for the purposes of layout" but I guess that'd have to be cross-compiler/etc/etc... probably too much work)

Once upon a time, C++ said that POD types could be memcpy'd, even if they were used as base classes. That rule got changed retroactively by core language defect reports, and you're no longer allowed to memcpy base class subobjects (nor [[no_unique_address]] members), but too late for the ABI, so as a consequence of a rule that we're now pretending never existed, the Itanium ABI is careful not to reuse the tail padding of a base class for members of the derived class if the base class is "POD for the purpose of layout" (which means POD according to some specific old C++ standard version).

An attribute to turn this off might make sense, for classes that really care. But so would an ABI-affecting flag to request that we always permit tail padding reuse. I do wonder if Clang should have a flag for "give me the best ABI you can, I don't care if it's non-standard; I'm building the whole world this way with this version of this compiler", just like libc++ does.

What's the cost of pair having non-trivial copy assignment? More expensive vector of pair resizing (though I guess the copy ctor calls would be readily optimized away & LLVM could get that back to a memcpy anyway?)?

Yes, this is probably only significant for code that's detecting the triviality and taking different action based on it. Clang has code in IR generation that tries to emit the actual copy assignment operator as a single memcpy regardless.

(the original issue with the warning could be fixed by narrowing the warning to only worry about trivial copy construction, not trivial copyability in general)

That sounds right to me. Is it worth also considering the triviality of the destructor?
 
- Dave 

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: std::pair not trivially copyable?

Hubert Tong via cfe-dev

Once upon a time, C++ said that POD types could be memcpy'd, even if they were used as base classes. That rule got changed retroactively by core language defect reports, and you're no longer allowed to memcpy base class subobjects (nor [[no_unique_address]] members), but too late for the ABI, so as a consequence of a rule that we're now pretending never existed, the Itanium ABI is careful not to reuse the tail padding of a base class for members of the derived class if the base class is "POD for the purpose of layout" (which means POD according to some specific old C++ standard version).

 

As an aside, I believe PS4 got caught by something along these lines; some combination of (the rule changed) and (Clang had a bug) was in an unfortunate state right at the time we had to nail down our ABI.  We’ve never gotten up the gumption to post a patch upstream, though.

 

IIUC (which is not super likely but I’ll give it a shot), pair is never trivially copyable, even with trivially copyable members.  Which seems sad.  Both pair and tuple have rules that say the destructor is trivial if the members are trivially destructible; seems like an oversight not to make triviality just as transitive for construct/move/copy.  Unless that ABI/tail-padding thing is what gets in the way.

--paulr

 

 

From: cfe-dev <[hidden email]> On Behalf Of Richard Smith via cfe-dev
Sent: Wednesday, August 26, 2020 9:24 PM
To: David Blaikie <[hidden email]>
Cc: [hidden email]
Subject: Re: [cfe-dev] std::pair not trivially copyable?

 

On Wed, 26 Aug 2020 at 11:42, David Blaikie <[hidden email]> wrote:

On Wed, Aug 26, 2020 at 11:34 AM Richard Smith via cfe-dev <[hidden email]> wrote:

On Wed, 26 Aug 2020 at 10:18, Nevin Liber via cfe-dev <[hidden email]> wrote:

On Tue, Aug 25, 2020 at 4:37 PM David Blaikie via cfe-dev <[hidden email]> wrote:

 

From what I can tell, reading this ( https://stackoverflow.com/questions/58283694/why-is-pair-of-const-trivially-copyable-but-pair-is-not ) and the C++17 spec, it just doesn't specify the copy and move assignment operators as defaulted, or say anything about their triviality, so they aren't trivial even for trivial types. Perhaps an oversight when thinking about the other complexities of when they shuold be deleted. 

 

In general, move/copy assignment cannot be defaulted for pair, because assignment of reference types has meaning:

 

int i = 2;

int j = 3;

std::pair<int&, int> p1(i, 5); 

std::pair<int&, int> p2(j, 7);

p2 = p1;  // j now has the value 2

 

 

struct A

{

    A(int& r_, int i_) : r(r_), i(i_) {}

    int& r;

    int i;

};

 

int i = 2;

int j = 3;

A a1(i, 5);

A a2(j, 7);

a2 = a1;  // Compile time error - deleted copy assignment operator

 

 

Now,  this doesn't that pair couldn't have trivial copy/move assignment operators when it holds trivially copy/move assignable types, but I don't know how much of an ABI break this would be.

 

Making std::pair be trivially-copyable whenever possible would affect whether std::pair is POD for the purpose of layout, which could result in an ABI break for at least any code that derives from std::pair or that contains a [[no_unique_address]] std::pair member: https://godbolt.org/z/GTvo4r

 

This is certainly not something we could do for the libc++ stable ABI. Maybe for the unstable ABI, but given the above change only makes layout worse, it's not clear that there would be a strong motivation. We already give std::pair trivial copy/move construction and trivial destruction whenever possible, so we can pass and return it efficiently.


Interesting - so POD for the purpose of layout is... not good? or more restrictive on the layout than if not. (probably an impractical idea, but would it be reasonable then to have an attribute that says "not POD for the purposes of layout" but I guess that'd have to be cross-compiler/etc/etc... probably too much work)

 

Once upon a time, C++ said that POD types could be memcpy'd, even if they were used as base classes. That rule got changed retroactively by core language defect reports, and you're no longer allowed to memcpy base class subobjects (nor [[no_unique_address]] members), but too late for the ABI, so as a consequence of a rule that we're now pretending never existed, the Itanium ABI is careful not to reuse the tail padding of a base class for members of the derived class if the base class is "POD for the purpose of layout" (which means POD according to some specific old C++ standard version).

 

An attribute to turn this off might make sense, for classes that really care. But so would an ABI-affecting flag to request that we always permit tail padding reuse. I do wonder if Clang should have a flag for "give me the best ABI you can, I don't care if it's non-standard; I'm building the whole world this way with this version of this compiler", just like libc++ does.

 

What's the cost of pair having non-trivial copy assignment? More expensive vector of pair resizing (though I guess the copy ctor calls would be readily optimized away & LLVM could get that back to a memcpy anyway?)?

 

Yes, this is probably only significant for code that's detecting the triviality and taking different action based on it. Clang has code in IR generation that tries to emit the actual copy assignment operator as a single memcpy regardless.

 

(the original issue with the warning could be fixed by narrowing the warning to only worry about trivial copy construction, not trivial copyability in general)

 

That sounds right to me. Is it worth also considering the triviality of the destructor?

 

- Dave 


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: std::pair not trivially copyable?

Hubert Tong via cfe-dev


On Thu, Aug 27, 2020 at 8:29 AM Robinson, Paul <[hidden email]> wrote:

Once upon a time, C++ said that POD types could be memcpy'd, even if they were used as base classes. That rule got changed retroactively by core language defect reports, and you're no longer allowed to memcpy base class subobjects (nor [[no_unique_address]] members), but too late for the ABI, so as a consequence of a rule that we're now pretending never existed, the Itanium ABI is careful not to reuse the tail padding of a base class for members of the derived class if the base class is "POD for the purpose of layout" (which means POD according to some specific old C++ standard version).

 

As an aside, I believe PS4 got caught by something along these lines; some combination of (the rule changed) and (Clang had a bug) was in an unfortunate state right at the time we had to nail down our ABI.  We’ve never gotten up the gumption to post a patch upstream, though.

 

IIUC (which is not super likely but I’ll give it a shot), pair is never trivially copyable, even with trivially copyable members. 


Yes, but with nuance: It's true that "is_trivially_copyable" will be false, but that requires both copy ctor and move ctor to be trivially copyable. It's specifically the copy assignment operator that is non-trivial. Pair is trivially copy /constructible/ when its members are.
 

Which seems sad.  Both pair and tuple have rules that say the destructor is trivial if the members are trivially destructible; seems like an oversight not to make triviality just as transitive for construct/move/copy.  

Unless that ABI/tail-padding thing is what gets in the way.


If I understand correctly its two layered:

1) making the copy assignment operator pass through triviality would be an ABI break today (but, hey, we have the libc++ unstable ABI option for this sort of reason, so if that were the only reason - it might be feasible (not sure what the C++ spec would have to say about this - perhaps it would need words to sanction this behavior))
2) but if we did that, it would make some objects sizeof larger, which would be bad/not-so-good, unless we had some extra attribute saying "it's POD, but not POD for the purposes of layout", or rather than an attribute, just a whole mode of "I'm building all C++ with this compiler - use the best ABI you can, with no concerns for compatibility with past/future compiler versions, or other compiler vendors", which would also allow the reuse of tail padding & thus the triviality to be pure goodness, rather than an awkward tradeoff
 

--paulr

 

 

From: cfe-dev <[hidden email]> On Behalf Of Richard Smith via cfe-dev
Sent: Wednesday, August 26, 2020 9:24 PM
To: David Blaikie <[hidden email]>
Cc: [hidden email]
Subject: Re: [cfe-dev] std::pair not trivially copyable?

 

On Wed, 26 Aug 2020 at 11:42, David Blaikie <[hidden email]> wrote:

On Wed, Aug 26, 2020 at 11:34 AM Richard Smith via cfe-dev <[hidden email]> wrote:

On Wed, 26 Aug 2020 at 10:18, Nevin Liber via cfe-dev <[hidden email]> wrote:

On Tue, Aug 25, 2020 at 4:37 PM David Blaikie via cfe-dev <[hidden email]> wrote:

 

From what I can tell, reading this ( https://stackoverflow.com/questions/58283694/why-is-pair-of-const-trivially-copyable-but-pair-is-not ) and the C++17 spec, it just doesn't specify the copy and move assignment operators as defaulted, or say anything about their triviality, so they aren't trivial even for trivial types. Perhaps an oversight when thinking about the other complexities of when they shuold be deleted. 

 

In general, move/copy assignment cannot be defaulted for pair, because assignment of reference types has meaning:

 

int i = 2;

int j = 3;

std::pair<int&, int> p1(i, 5); 

std::pair<int&, int> p2(j, 7);

p2 = p1;  // j now has the value 2

 

 

struct A

{

    A(int& r_, int i_) : r(r_), i(i_) {}

    int& r;

    int i;

};

 

int i = 2;

int j = 3;

A a1(i, 5);

A a2(j, 7);

a2 = a1;  // Compile time error - deleted copy assignment operator

 

 

Now,  this doesn't that pair couldn't have trivial copy/move assignment operators when it holds trivially copy/move assignable types, but I don't know how much of an ABI break this would be.

 

Making std::pair be trivially-copyable whenever possible would affect whether std::pair is POD for the purpose of layout, which could result in an ABI break for at least any code that derives from std::pair or that contains a [[no_unique_address]] std::pair member: https://godbolt.org/z/GTvo4r

 

This is certainly not something we could do for the libc++ stable ABI. Maybe for the unstable ABI, but given the above change only makes layout worse, it's not clear that there would be a strong motivation. We already give std::pair trivial copy/move construction and trivial destruction whenever possible, so we can pass and return it efficiently.


Interesting - so POD for the purpose of layout is... not good? or more restrictive on the layout than if not. (probably an impractical idea, but would it be reasonable then to have an attribute that says "not POD for the purposes of layout" but I guess that'd have to be cross-compiler/etc/etc... probably too much work)

 

Once upon a time, C++ said that POD types could be memcpy'd, even if they were used as base classes. That rule got changed retroactively by core language defect reports, and you're no longer allowed to memcpy base class subobjects (nor [[no_unique_address]] members), but too late for the ABI, so as a consequence of a rule that we're now pretending never existed, the Itanium ABI is careful not to reuse the tail padding of a base class for members of the derived class if the base class is "POD for the purpose of layout" (which means POD according to some specific old C++ standard version).

 

An attribute to turn this off might make sense, for classes that really care. But so would an ABI-affecting flag to request that we always permit tail padding reuse. I do wonder if Clang should have a flag for "give me the best ABI you can, I don't care if it's non-standard; I'm building the whole world this way with this version of this compiler", just like libc++ does.


Certainly crossed my (& you & other folks) mind, for sure. Would we be able to use that at Google? (do we have a strong C++ ABI boundary, or just a strong C++ standard library ABI boundary?) (apologies if this is a bit too internal to discuss here, but seems relevant enough & non-proprietary) 

What's the cost of pair having non-trivial copy assignment? More expensive vector of pair resizing (though I guess the copy ctor calls would be readily optimized away & LLVM could get that back to a memcpy anyway?)?

 

Yes, this is probably only significant for code that's detecting the triviality and taking different action based on it. Clang has code in IR generation that tries to emit the actual copy assignment operator as a single memcpy regardless.


Oh, nice! Didn't know that triggered even when technically non-trivial.

(the original issue with the warning could be fixed by narrowing the warning to only worry about trivial copy construction, not trivial copyability in general)

 

That sounds right to me. Is it worth also considering the triviality of the destructor?


Maybe? I'm OK either way on that. Not sure who wrote the original/would be interested in fixing it in some way. +Sam McCall in case he's interested/can point the right people at this

- Dave

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: std::pair not trivially copyable?

Hubert Tong via cfe-dev
In reply to this post by Hubert Tong via cfe-dev
On Wed, Aug 26, 2020 at 1:34 PM Richard Smith <[hidden email]> wrote:
This is certainly not something we could do for the libc++ stable ABI. Maybe for the unstable ABI, but given the above change only makes layout worse, it's not clear that there would be a strong motivation. We already give std::pair trivial copy/move construction and trivial destruction whenever possible, so we can pass and return it efficiently.

Our motivation is that in the heterogeneous computing world (for instance in SYCL/DPC++), trivially copyable is used to copy things between hosts and devices (CPU and GPU).

There is an expectation that standard library wrapper types (pair, tuple, optional, variant, etc.) don't break trivialness.  This is especially true of pair, which most people expect to be "just a struct" of two public types.
--
 Nevin ":-)" Liber  <mailto:[hidden email]>  +1-847-691-1404

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: std::pair not trivially copyable?

Hubert Tong via cfe-dev
In reply to this post by Hubert Tong via cfe-dev

Hopefully without diverting the quite relevant discussion of std::pair’s triviality, and looking at this Blaikie/Smith exchange again:

 

(the original issue with the warning could be fixed by narrowing the warning to only worry about trivial copy construction, not trivial copyability in general)

 

That sounds right to me. Is it worth also considering the triviality of the destructor?

 

Looking at the description of range-for here https://en.cppreference.com/w/cpp/language/range-for it has

               range_declaration = *__begin;

…which is an initialization, not an assignment; so for the diagnostic (and codegen), Clang should be looking at copy-initialization/construction not copy-assignment anyway.  And std::pair’s copy constructors *are* trivial.

Right?

--paulr

 

From: cfe-dev <[hidden email]> On Behalf Of Richard Smith via cfe-dev
Sent: Wednesday, August 26, 2020 9:24 PM
To: David Blaikie <[hidden email]>
Cc: [hidden email]
Subject: Re: [cfe-dev] std::pair not trivially copyable?

 

On Wed, 26 Aug 2020 at 11:42, David Blaikie <[hidden email]> wrote:

On Wed, Aug 26, 2020 at 11:34 AM Richard Smith via cfe-dev <[hidden email]> wrote:

On Wed, 26 Aug 2020 at 10:18, Nevin Liber via cfe-dev <[hidden email]> wrote:

On Tue, Aug 25, 2020 at 4:37 PM David Blaikie via cfe-dev <[hidden email]> wrote:

 

From what I can tell, reading this ( https://stackoverflow.com/questions/58283694/why-is-pair-of-const-trivially-copyable-but-pair-is-not ) and the C++17 spec, it just doesn't specify the copy and move assignment operators as defaulted, or say anything about their triviality, so they aren't trivial even for trivial types. Perhaps an oversight when thinking about the other complexities of when they shuold be deleted. 

 

In general, move/copy assignment cannot be defaulted for pair, because assignment of reference types has meaning:

 

int i = 2;

int j = 3;

std::pair<int&, int> p1(i, 5); 

std::pair<int&, int> p2(j, 7);

p2 = p1;  // j now has the value 2

 

 

struct A

{

    A(int& r_, int i_) : r(r_), i(i_) {}

    int& r;

    int i;

};

 

int i = 2;

int j = 3;

A a1(i, 5);

A a2(j, 7);

a2 = a1;  // Compile time error - deleted copy assignment operator

 

 

Now,  this doesn't that pair couldn't have trivial copy/move assignment operators when it holds trivially copy/move assignable types, but I don't know how much of an ABI break this would be.

 

Making std::pair be trivially-copyable whenever possible would affect whether std::pair is POD for the purpose of layout, which could result in an ABI break for at least any code that derives from std::pair or that contains a [[no_unique_address]] std::pair member: https://godbolt.org/z/GTvo4r

 

This is certainly not something we could do for the libc++ stable ABI. Maybe for the unstable ABI, but given the above change only makes layout worse, it's not clear that there would be a strong motivation. We already give std::pair trivial copy/move construction and trivial destruction whenever possible, so we can pass and return it efficiently.


Interesting - so POD for the purpose of layout is... not good? or more restrictive on the layout than if not. (probably an impractical idea, but would it be reasonable then to have an attribute that says "not POD for the purposes of layout" but I guess that'd have to be cross-compiler/etc/etc... probably too much work)

 

Once upon a time, C++ said that POD types could be memcpy'd, even if they were used as base classes. That rule got changed retroactively by core language defect reports, and you're no longer allowed to memcpy base class subobjects (nor [[no_unique_address]] members), but too late for the ABI, so as a consequence of a rule that we're now pretending never existed, the Itanium ABI is careful not to reuse the tail padding of a base class for members of the derived class if the base class is "POD for the purpose of layout" (which means POD according to some specific old C++ standard version).

 

An attribute to turn this off might make sense, for classes that really care. But so would an ABI-affecting flag to request that we always permit tail padding reuse. I do wonder if Clang should have a flag for "give me the best ABI you can, I don't care if it's non-standard; I'm building the whole world this way with this version of this compiler", just like libc++ does.

 

What's the cost of pair having non-trivial copy assignment? More expensive vector of pair resizing (though I guess the copy ctor calls would be readily optimized away & LLVM could get that back to a memcpy anyway?)?

 

Yes, this is probably only significant for code that's detecting the triviality and taking different action based on it. Clang has code in IR generation that tries to emit the actual copy assignment operator as a single memcpy regardless.

 

(the original issue with the warning could be fixed by narrowing the warning to only worry about trivial copy construction, not trivial copyability in general)

 

That sounds right to me. Is it worth also considering the triviality of the destructor?

 

- Dave 


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: std::pair not trivially copyable?

Hubert Tong via cfe-dev
In reply to this post by Hubert Tong via cfe-dev
On Wed, Aug 26, 2020 at 1:34 PM Richard Smith <[hidden email]> wrote:
Making std::pair be trivially-copyable whenever possible would affect whether std::pair is POD for the purpose of layout, which could result in an ABI break for at least any code that derives from std::pair or that contains a [[no_unique_address]] std::pair member: https://godbolt.org/z/GTvo4r

Given Key and T are trivially copyable types:

pair<const Key, T> is trivially copyable.
pair<Key, T> is not trivially copyable.

I find this surprising.


And if we look at node handles for associative containers:

In [container.node.overview]p4 the standard says: "If a user-defined specialization of pair exists for pair<const Key, T> or pair<Key, T>, where Key is the container's key_­type and T is the container's mapped_­type, the behavior of operations involving node handles is undefined."

I presumed that was done so standard library implementations could type pun between pair<const Key, T> and pair<Key, T> in their implementations of node handles.

Is this just not a problem in practice?
--
 Nevin ":-)" Liber  <mailto:[hidden email]>  +1-847-691-1404

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: std::pair not trivially copyable?

Hubert Tong via cfe-dev
In reply to this post by Hubert Tong via cfe-dev


On Mon, Aug 31, 2020 at 8:09 AM Robinson, Paul <[hidden email]> wrote:

Hopefully without diverting the quite relevant discussion of std::pair’s triviality, and looking at this Blaikie/Smith exchange again:

 

(the original issue with the warning could be fixed by narrowing the warning to only worry about trivial copy construction, not trivial copyability in general)

 

That sounds right to me. Is it worth also considering the triviality of the destructor?

 

Looking at the description of range-for here https://en.cppreference.com/w/cpp/language/range-for it has

               range_declaration = *__begin;

…which is an initialization, not an assignment; so for the diagnostic (and codegen), Clang should be looking at copy-initialization/construction not copy-assignment anyway.  And std::pair’s copy constructors *are* trivial.

Right?


Right - hence the suggestion. The question about whether non-trivial dtor should matter to the warning is a fair one & I don't have a good sense of the right answer - would be OK with that causing the warning too (though that'd add (a probably small number - probably aren't many things with a non-trivial dtor, but a trivial copy ctor) of new warnings).
 

--paulr

 

From: cfe-dev <[hidden email]> On Behalf Of Richard Smith via cfe-dev
Sent: Wednesday, August 26, 2020 9:24 PM
To: David Blaikie <[hidden email]>
Cc: [hidden email]
Subject: Re: [cfe-dev] std::pair not trivially copyable?

 

On Wed, 26 Aug 2020 at 11:42, David Blaikie <[hidden email]> wrote:

On Wed, Aug 26, 2020 at 11:34 AM Richard Smith via cfe-dev <[hidden email]> wrote:

On Wed, 26 Aug 2020 at 10:18, Nevin Liber via cfe-dev <[hidden email]> wrote:

On Tue, Aug 25, 2020 at 4:37 PM David Blaikie via cfe-dev <[hidden email]> wrote:

 

From what I can tell, reading this ( https://stackoverflow.com/questions/58283694/why-is-pair-of-const-trivially-copyable-but-pair-is-not ) and the C++17 spec, it just doesn't specify the copy and move assignment operators as defaulted, or say anything about their triviality, so they aren't trivial even for trivial types. Perhaps an oversight when thinking about the other complexities of when they shuold be deleted. 

 

In general, move/copy assignment cannot be defaulted for pair, because assignment of reference types has meaning:

 

int i = 2;

int j = 3;

std::pair<int&, int> p1(i, 5); 

std::pair<int&, int> p2(j, 7);

p2 = p1;  // j now has the value 2

 

 

struct A

{

    A(int& r_, int i_) : r(r_), i(i_) {}

    int& r;

    int i;

};

 

int i = 2;

int j = 3;

A a1(i, 5);

A a2(j, 7);

a2 = a1;  // Compile time error - deleted copy assignment operator

 

 

Now,  this doesn't that pair couldn't have trivial copy/move assignment operators when it holds trivially copy/move assignable types, but I don't know how much of an ABI break this would be.

 

Making std::pair be trivially-copyable whenever possible would affect whether std::pair is POD for the purpose of layout, which could result in an ABI break for at least any code that derives from std::pair or that contains a [[no_unique_address]] std::pair member: https://godbolt.org/z/GTvo4r

 

This is certainly not something we could do for the libc++ stable ABI. Maybe for the unstable ABI, but given the above change only makes layout worse, it's not clear that there would be a strong motivation. We already give std::pair trivial copy/move construction and trivial destruction whenever possible, so we can pass and return it efficiently.


Interesting - so POD for the purpose of layout is... not good? or more restrictive on the layout than if not. (probably an impractical idea, but would it be reasonable then to have an attribute that says "not POD for the purposes of layout" but I guess that'd have to be cross-compiler/etc/etc... probably too much work)

 

Once upon a time, C++ said that POD types could be memcpy'd, even if they were used as base classes. That rule got changed retroactively by core language defect reports, and you're no longer allowed to memcpy base class subobjects (nor [[no_unique_address]] members), but too late for the ABI, so as a consequence of a rule that we're now pretending never existed, the Itanium ABI is careful not to reuse the tail padding of a base class for members of the derived class if the base class is "POD for the purpose of layout" (which means POD according to some specific old C++ standard version).

 

An attribute to turn this off might make sense, for classes that really care. But so would an ABI-affecting flag to request that we always permit tail padding reuse. I do wonder if Clang should have a flag for "give me the best ABI you can, I don't care if it's non-standard; I'm building the whole world this way with this version of this compiler", just like libc++ does.

 

What's the cost of pair having non-trivial copy assignment? More expensive vector of pair resizing (though I guess the copy ctor calls would be readily optimized away & LLVM could get that back to a memcpy anyway?)?

 

Yes, this is probably only significant for code that's detecting the triviality and taking different action based on it. Clang has code in IR generation that tries to emit the actual copy assignment operator as a single memcpy regardless.

 

(the original issue with the warning could be fixed by narrowing the warning to only worry about trivial copy construction, not trivial copyability in general)

 

That sounds right to me. Is it worth also considering the triviality of the destructor?

 

- Dave 


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev