[cfe-dev] std::pair not trivially copyable?

[David Blaikie via cfe-dev]

On Thu, Aug 27, 2020 at 8:29 AM Robinson, Paul <paul.robinson at sony.com>
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 <cfe-dev-bounces at lists.llvm.org> *On Behalf Of *Richard
> Smith via cfe-dev
> *Sent:* Wednesday, August 26, 2020 9:24 PM
> *To:* David Blaikie <dblaikie at gmail.com>
> *Cc:* cfe-dev at lists.llvm.org
> *Subject:* Re: [cfe-dev] std::pair not trivially copyable?
>
>
>
> On Wed, 26 Aug 2020 at 11:42, David Blaikie <dblaikie at gmail.com> wrote:
>
> On Wed, Aug 26, 2020 at 11:34 AM Richard Smith via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
> On Wed, 26 Aug 2020 at 10:18, Nevin Liber via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
> On Tue, Aug 25, 2020 at 4:37 PM David Blaikie via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>
>
> From what I can tell, reading this (
> https://stackoverflow.com/questions/58283694/why-is-pair-of-const-trivially-copyable-but-pair-is-not
> <https://urldefense.com/v3/__https:/stackoverflow.com/questions/58283694/why-is-pair-of-const-trivially-copyable-but-pair-is-not__;!!JmoZiZGBv3RvKRSx!rMuXOBoosPAUEb_L7l9qA3KHSuisIvxQH9YC3nw5t0zEC7c5K6WEpAqMrB47qTd29A$> )
> 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
> <https://urldefense.com/v3/__https:/godbolt.org/z/GTvo4r__;!!JmoZiZGBv3RvKRSx!rMuXOBoosPAUEb_L7l9qA3KHSuisIvxQH9YC3nw5t0zEC7c5K6WEpAqMrB6_XykF_w$>
>
>
>
> 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

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200827/9a9c59ab/attachment.html>