[Bug libstdc++/53657] New: [C++11] pair(pair&&) move constructor is non-trivial

[Bug libstdc++/53657] New: [C++11] pair(pair&&) move constructor is non-trivial

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

             Bug #: 53657
           Summary: [C++11] pair(pair&&) move constructor is non-trivial
    Classification: Unclassified
           Product: gcc
           Version: 4.7.1
            Status: UNCONFIRMED
          Keywords: ABI
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: redi@gcc.gnu.org
                CC: paolo.carlini@oracle.com


As discussed in PR 53646 the std::pair move constructor is non-trivial and so
causes an unfortunate ABI change where std::pair gets returned from functions
by invisible reference instead of in registers.

Defaulting it makes it trivial, but causes other problems:

      // XXX Defaulted?!? Breaks std::map!!!
      pair(pair&& __p)
      noexcept(__and_<is_nothrow_move_constructible<_T1>,
                      is_nothrow_move_constructible<_T2>>::value)
      : first(std::forward<first_type>(__p.first)),
        second(std::forward<second_type>(__p.second)) { }

[Bug libstdc++/53657] [C++11] pair(pair&&) move constructor is non-trivial

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-06-14
                 CC|                            |rguenth at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-06-14 08:44:49 UTC ---
Can you list the known "broken" revisions?  I'm not sure what to best do, but
eventually fixing it even when that "breaks" the ABI on release branches might
be the best thing after all ...

[Bug libstdc++/53657] [C++11] pair(pair&&) move constructor is non-trivial

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |4.7.0, 4.7.1, 4.8.0

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-06-14 09:15:02 UTC ---
I'm not sure if G++ uses an invisible register for it on the 4,7 branch, but
the move-ctor is definitely non-trivial on the 4.7 branch and trunk.  It's
trivial on the 4.6 branch.

[Bug libstdc++/53657] [C++11] pair(pair&&) move constructor is non-trivial

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-06-14 09:17:51 UTC ---
it was also non-trivial in 4.5.0, but not 4.5.1+

[Bug libstdc++/53657] [4.7/4.8 Regression][C++11] pair(pair&&) move constructor is non-trivial

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |4.5.1, 4.6.0
   Target Milestone|---                         |4.7.2
            Summary|[C++11] pair(pair&&) move   |[4.7/4.8 Regression][C++11]
                   |constructor is non-trivial  |pair(pair&&) move
                   |                            |constructor is non-trivial
      Known to fail|                            |4.5.0

--- Comment #4 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-06-14 09:39:58 UTC ---
Thanks.

[Bug libstdc++/53657] [C++11] pair(pair&&) move constructor is non-trivial

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |4.5.1, 4.6.0
   Target Milestone|---                         |4.7.2
            Summary|[C++11] pair(pair&&) move   |[4.7/4.8 Regression][C++11]
                   |constructor is non-trivial  |pair(pair&&) move
                   |                            |constructor is non-trivial
      Known to fail|                            |4.5.0

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|4.5.1, 4.6.0                |
   Target Milestone|4.7.2                       |---
            Summary|[4.7/4.8 Regression][C++11] |[C++11] pair(pair&&) move
                   |pair(pair&&) move           |constructor is non-trivial
                   |constructor is non-trivial  |
      Known to fail|4.5.0                       |

--- Comment #4 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-06-14 09:39:58 UTC ---
Thanks.

--- Comment #5 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-06-14 09:40:44 UTC ---
The issue is related to LWG2005, and can be summarized with the following
testcase, which should not trigger the static_assert.

In short, if we want std::pair' move constructor defaulted, we have to use
std::is_constructible, not std::is_convertible, to constrain container::insert.

If people agree with my assessment - I don't see what else we could possibly do
on the library side - I can try again moving the latter to use
std::is_constructible, should mostly work.

/////////////////

#include <type_traits>
#include <utility>

struct move_only
{
  move_only(const move_only&) = delete;
  move_only(move_only&&) = default;
};

template<typename _T1, typename _T2>
 struct pair
 {
   pair(pair&&) = default;

   template<class _U1, class _U2>
     pair(pair<_U1, _U2>&& __p)
     : first(std::forward<_U1>(__p.first)),
       second(std::forward<_U2>(__p.second)) { }

   _T1 first;
   _T2 second;
 };

typedef pair<move_only, move_only> Pair;
typedef pair<const move_only, move_only> CPair;

static_assert(std::is_convertible<Pair, CPair>::value, "Error");
//static_assert(std::is_constructible<CPair, Pair&&>::value, "Error");

[Bug libstdc++/53657] [C++11] pair(pair&&) move constructor is non-trivial

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org

--- Comment #6 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-06-14 09:43:04 UTC ---
Let's add Jason in CC for this one too, just in case he can see something in
the front-end area too.

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |4.5.1, 4.6.1
   Target Milestone|---                         |4.7.2
            Summary|[C++11] pair(pair&&) move   |[4.7/4.8 Regression]
                   |constructor is non-trivial  |[C++11] pair(pair&&) move
                   |                            |constructor is non-trivial
      Known to fail|                            |4.5.0

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |org.gnu.gcc.bugtracker at
                   |                            |sotecware dot net

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-06-14 13:15:30 UTC ---
*** Bug 53455 has been marked as a duplicate of this bug. ***

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|jason at gcc dot gnu.org,   |
                   |paolo.carlini at oracle dot |
                   |com                         |
         AssignedTo|unassigned at gcc dot       |paolo.carlini at oracle dot
                   |gnu.org                     |com

--- Comment #8 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-06-19 09:30:00 UTC ---
Ah, great, Daniel pointed out in private mail that the latest update of LWG
2005 indeed specifies is_constructible as I seemed to remember:

  http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#2005

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jwakely.gcc at gmail dot
                   |                            |com

--- Comment #9 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-06-20 15:04:11 UTC ---
If I do the straightforward change (pair(pair&&) defaulted + insert(_Pair&&)
using is_constructible), there are problems in the testsuite for situations
well exemplified by 23_containers/unordered_map/insert/map_range.cc (many
instances in the exception/ testcases too and elsewhere, but hidden below a lot
of templates, grrr): the defaulted pair move constructor is implicitly deleted
and the testcase doesn't compile. Is that expected and the testcase defective?
Maybe yes. We have things like:

  typedef std::pair<const std::string, int> Pair;

  Pair A = Pair("red", 5);

if I remove the const from Pair it works.

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[daniel.kruegler at googlemail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

--- Comment #10 from Daniel Krügler <daniel.kruegler at googlemail dot com> 2012-06-20 15:10:16 UTC ---
(In reply to comment #9)
Yes, this is to be expected, because gcc currently does not implement

http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1402

I think fixing that issue depends on that DR (Sorry, I forgot to mention that
in my private communication to you).

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

--- Comment #11 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-06-20 15:15:46 UTC ---
Ah, thanks Daniel. Thus I guess we should do this change and then also
implement core/1402 ASAP, because otherwise in many circumstances the users
would not be that happy. Are you willing to open a GCC Bugzilla with Jason too
in CC about core/1402 and referring to this one?

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-06-20 15:20:29 UTC ---
Possibly a front end bug, not sure.

Here's a reduced form that G++ rejects, Clang accepts:

template<typename T>
struct pair
{
    constexpr pair(const pair&) = default;
    pair(pair&&) = default;

    pair() : t() { }

    T t;
};

struct string {
    string(const string&) { }
    string(string&&) = default;
    string() = default;
};

int main()
{
    typedef pair<const string> P;
    P p[] = { P(), P() };
}

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

--- Comment #13 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-06-20 15:23:06 UTC ---
By the way, the more I look into this issue the more I come to the conclusion
that making sure pair(pair&&) is defaulted (which has ABI implications) means
doing quite a number of non-trivial changes both in lib and front-end beyond
the letter of C++11. I vaguely knew that for a while, guys...

To good news is that defaulting + insert(Pair&&) using is_constructible + DR
1402 in the front-end should lead to a clean library testsuite, I don't see any
other issues when regression testing the first 2 items.

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

--- Comment #14 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-06-20 15:26:06 UTC ---
Thanks for the no-include testcase Jon. Thus are we talking about the core
defect mentioned by Daniel? Should move this discussion to a C++ issue.

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[daniel.kruegler at googlemail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

--- Comment #14 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-06-20 15:26:06 UTC ---
Thanks for the no-include testcase Jon. Thus are we talking about the core
defect mentioned by Daniel? Should move this discussion to a C++ issue.

--- Comment #15 from Daniel Krügler <daniel.kruegler at googlemail dot com> 2012-06-20 15:26:52 UTC ---
(In reply to comment #11)
> Are you willing to open a GCC Bugzilla with Jason too
> in CC about core/1402 and referring to this one?

Yes, I do that this evening.

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

--- Comment #16 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-06-20 15:30:39 UTC ---
(In reply to comment #14)
> Thanks for the no-include testcase Jon. Thus are we talking about the core
> defect mentioned by Daniel? Should move this discussion to a C++ issue.

Yes, same issue, I'd just written it before I saw the replies and got a mid-air
collision, so I hit "submit only my new comment".

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #17 from paolo at gcc dot gnu.org <paolo at gcc dot gnu.org> 2012-07-13 09:00:05 UTC ---
Author: paolo
Date: Fri Jul 13 08:59:58 2012
New Revision: 189456

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=189456
Log:
2012-07-13  Paolo Carlini  <paolo.carlini@oracle.com>

    PR libstdc++/53657
    * include/bits/stl_pair.h (pair<>::pair(pair&&)): Declare defaulted,
    per C++11.
    * include/bits/stl_map.h (map<>::insert(_Pair&&), map<>::insert
    (const_iterator, _Pair&&)): Constrain with std::is_constructible,
    per LWG2005.
    * include/bits/stl_multimap.h (multimap<>::insert(_Pair&&),
    multimap<>::insert(const_iterator, _Pair&&)): Likewise.
    * include/bits/hashtable_policy.h (_Insert<>::insert(_Pair&&),
    _Insert<>::insert(const_iterator, _Pair&&)): Likewise.
    * include/debug/unordered_map: Adjust.
    * include/debug/map.h: Likewise.
    * include/debug/multimap.h: Likewise.
    * include/profile/unordered_map: Likewise.
    * include/profile/map.h: Likewise.
    * include/profile/multimap.h: Likewise.


Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/hashtable_policy.h
    trunk/libstdc++-v3/include/bits/stl_map.h
    trunk/libstdc++-v3/include/bits/stl_multimap.h
    trunk/libstdc++-v3/include/bits/stl_pair.h
    trunk/libstdc++-v3/include/debug/map.h
    trunk/libstdc++-v3/include/debug/multimap.h
    trunk/libstdc++-v3/include/debug/unordered_map
    trunk/libstdc++-v3/include/profile/map.h
    trunk/libstdc++-v3/include/profile/multimap.h
    trunk/libstdc++-v3/include/profile/unordered_map

--- Comment #18 from paolo at gcc dot gnu.org <paolo at gcc dot gnu.org> 2012-07-13 09:00:24 UTC ---
Author: paolo
Date: Fri Jul 13 09:00:18 2012
New Revision: 189457

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=189457
Log:
2012-07-13  Paolo Carlini  <paolo.carlini@oracle.com>

    PR libstdc++/53657
    * include/bits/stl_pair.h (pair<>::pair(pair&&)): Declare defaulted,
    per C++11.
    * include/bits/stl_map.h (map<>::insert(_Pair&&), map<>::insert
    (const_iterator, _Pair&&)): Constrain with std::is_constructible,
    per LWG2005.
    * include/bits/stl_multimap.h (multimap<>::insert(_Pair&&),
    multimap<>::insert(const_iterator, _Pair&&)): Likewise.
    * include/bits/hashtable.h (_Hashtable<>::insert(_Pair&&),
    _Hashtable<>::insert(const_iterator, _Pair&&)): Likewise.
    * include/debug/unordered_map: Adjust.
    * include/debug/map.h: Likewise.
    * include/debug/multimap.h: Likewise.
    * include/profile/unordered_map: Likewise.
    * include/profile/map.h: Likewise.
    * include/profile/multimap.h: Likewise.


Modified:
    branches/gcc-4_7-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_7-branch/libstdc++-v3/include/bits/hashtable.h
    branches/gcc-4_7-branch/libstdc++-v3/include/bits/stl_map.h
    branches/gcc-4_7-branch/libstdc++-v3/include/bits/stl_multimap.h
    branches/gcc-4_7-branch/libstdc++-v3/include/bits/stl_pair.h
    branches/gcc-4_7-branch/libstdc++-v3/include/debug/map.h
    branches/gcc-4_7-branch/libstdc++-v3/include/debug/multimap.h
    branches/gcc-4_7-branch/libstdc++-v3/include/debug/unordered_map
    branches/gcc-4_7-branch/libstdc++-v3/include/profile/map.h
    branches/gcc-4_7-branch/libstdc++-v3/include/profile/multimap.h
    branches/gcc-4_7-branch/libstdc++-v3/include/profile/unordered_map

--- Comment #19 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-07-13 09:04:33 UTC ---
Fixed mainline and 4.7.2.

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[bkoz at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

Benjamin Kosnik <bkoz at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bkoz at gcc dot gnu.org

--- Comment #20 from Benjamin Kosnik <bkoz at gcc dot gnu.org> 2012-07-13 12:38:03 UTC ---

Thanks Paolo and Jonathan and Jason, this looks ready to go from an ABI
standpoint for 4.7.2.

Paolo, do we need to add any of the testcases in this PR to track regressions
here? Or at least make this less fragile as FE/lib changes going forward?

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

--- Comment #21 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-07-13 12:43:31 UTC ---
I think that if something regresses we immediately see regressions in the v3
testsuite (Comment 5 is distilled from it).

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

--- Comment #22 from rguenther at suse dot de <rguenther at suse dot de> 2012-07-13 12:53:51 UTC ---
On Fri, 13 Jul 2012, paolo.carlini at oracle dot com wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657
> 
> --- Comment #21 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-07-13 12:43:31 UTC ---
> I think that if something regresses we immediately see regressions in the v3
> testsuite (Comment 5 is distilled from it).

Can one of you please add a note to gcc-4.7/changes.html to reflect
the ABI changes between 4.7.1 and 4.7.2?  Thanks.

[Bug libstdc++/53657] [4.7/4.8 Regression] [C++11] pair(pair&&) move constructor is non-trivial

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53657

--- Comment #23 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-07-15 15:07:25 UTC ---
(In reply to comment #22)
> Can one of you please add a note to gcc-4.7/changes.html to reflect
> the ABI changes between 4.7.1 and 4.7.2?  Thanks.

Done: http://gcc.gnu.org/ml/gcc-patches/2012-07/msg00584.html