[Bug libstdc++/103853] New: std::forward_list::merge should check if __list != this

[Bug libstdc++/103853] New: std::forward_list::merge should check if __list != this

[pavel.kryukov at phystech dot edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

            Bug ID: 103853
           Summary: std::forward_list::merge should check if __list !=
                    this
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: pavel.kryukov at phystech dot edu
  Target Milestone: ---

Created attachment 52080
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52080&action=edit
check if __list==this in std::forward_list::merge

C++ standard does not require 'do nothing' if std::forward_list::merge argument
equals to 'this', however it does not say 'undefined behavior' or 'infinite
loop' too.

I've attached my patch to fix and test it, but since I'm not an experienced GCC
contributor, I don't know how to run the tests. Following instructions on
CONTRIBUTING page builds whole GCC ending with some problems with my machine. I
would be grateful if you tell how I can test only libstdc++.

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-12-29
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Pavel I. Kryukov from comment #0)
> C++ standard does not require 'do nothing' if std::forward_list::merge
> argument equals to 'this', however it does not say 'undefined behavior' or
> 'infinite loop' too.

It does say undefined behaviour for the overload your patch changes.

[res.on.arguments] says:

> If a function argument is bound to an rvalue reference parameter, the
> implementation may assume that this parameter is a unique reference to this
> argument, except that the argument passed to a move-assignment operator may
> be a reference to *this ([lib.types.movedfrom]).

Passing *this as the first parameter of merge(forward_list&&) or
merge(forward_list&&, Comp) breaks this assumption, resulting in undefined
behaviour.

The check is only needed for the overloads taking forward_list& parameters:

--- a/libstdc++-v3/include/bits/forward_list.h
+++ b/libstdc++-v3/include/bits/forward_list.h
@@ -1267,7 +1267,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER

       void
       merge(forward_list& __list)
-      { merge(std::move(__list)); }
+      {
+       if (std::__addressof(__list) == this)
+         return;
+
+       merge(std::move(__list));
+      }

       /**
        *  @brief  Merge sorted lists according to comparison function.
@@ -1287,7 +1292,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       template<typename _Comp>
        void
        merge(forward_list& __list, _Comp __comp)
-       { merge(std::move(__list), __comp); }
+       {
+         if (std::__addressof(__list) == this)
+           return;
+
+         merge(std::move(__list), __comp);
+       }

       /**
        *  @brief  Sort the elements of the list.


> I've attached my patch to fix and test it, but since I'm not an experienced
> GCC contributor, I don't know how to run the tests. Following instructions
> on CONTRIBUTING page builds whole GCC ending with some problems with my
> machine. I would be grateful if you tell how I can test only libstdc++.

You need to build the whole of GCC to test libstdc++, so if you can't get that
working, you should fix that first.

Testing libstdc++ is documented at
https://gcc.gnu.org/onlinedocs/libstdc++/manual/test.html#test.run

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The new test should also check the overload taking a comparison function.

And if we do want to add the check for the rvalue overloads (which isn't
required, but might be more user friendly, with a small overhead) those should
be checked in the test as well.

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
N.B. this is https://cplusplus.github.io/LWG/issue3088

Currently the standard actually requires that x.merge(x) does x.clear() which
is obviously not what we want.

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Pavel, I'm going to use your original patch, or parts of it anyway (the
addition to the changelog is wrong, we do not patch the changelog by hand it's
auto-generated).

Are you willing and able to certify the origin of the code under the terms of
https://gcc.gnu.org/dco.html ?

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[pavel.kryukov at phystech dot edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

--- Comment #5 from Pavel I. Kryukov <pavel.kryukov at phystech dot edu> ---
> Are you willing and able to certify the origin of the code under the terms of https://gcc.gnu.org/dco.html ?

Yes. Do I need to resubmit a signed-off patch?

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I think I can probably just add the Signed-off-by: tag based on your comment
above, and push it. But it wouldn't hurt to attach a signed-off patch here.

Thanks!

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[pavel.kryukov at phystech dot edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

Pavel I. Kryukov <pavel.kryukov at phystech dot edu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #52080|0                           |1
        is obsolete|                            |

--- Comment #7 from Pavel I. Kryukov <pavel.kryukov at phystech dot edu> ---
Created attachment 52133
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52133&action=edit
check if __list==this in std::forward_list::merge

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:52ebc2be0990d6d3a46bb716164f9cef6f661021

commit r12-6284-g52ebc2be0990d6d3a46bb716164f9cef6f661021
Author: Pavel I. Kryukov <pavel.kryukov@phystech.edu>
Date:   Thu Jan 6 12:32:36 2022 +0000

    libstdc++: Add self-merge check to std::forward_list::merge [PR103853]

    This implements the proposed resolution of LWG 3088, so that x.merge(x)
    is a no-op, consistent with std::list::merge.

    Signed-off-by: Pavel I. Kryukov <pavel.kryukov@phystech.edu>

    Co-authored-by: Jonathan Wakely <jwakely@redhat.com>

    libstdc++-v3/ChangeLog:

            PR libstdc++/103853
            * include/bits/forward_list.tcc (forward_list::merge): Check for
            self-merge.
            * testsuite/23_containers/forward_list/operations/merge.cc: New
test.

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Thanks! This is fixed on trunk now, and will be backported to the release
branches soon.

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[pavel.kryukov at phystech dot edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

--- Comment #10 from Pavel I. Kryukov <pavel.kryukov at phystech dot edu> ---
Thanks a lot!

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:2089886d5743c20ce0b41d68f0a4cbe097b46704

commit r11-9909-g2089886d5743c20ce0b41d68f0a4cbe097b46704
Author: Pavel I. Kryukov <pavel.kryukov@phystech.edu>
Date:   Thu Jan 6 12:32:36 2022 +0000

    libstdc++: Add self-merge check to std::forward_list::merge [PR103853]

    This implements the proposed resolution of LWG 3088, so that x.merge(x)
    is a no-op, consistent with std::list::merge.

    Signed-off-by: Pavel I. Kryukov <pavel.kryukov@phystech.edu>

    Co-authored-by: Jonathan Wakely <jwakely@redhat.com>

    libstdc++-v3/ChangeLog:

            PR libstdc++/103853
            * include/bits/forward_list.tcc (forward_list::merge): Check for
            self-merge.
            * testsuite/23_containers/forward_list/operations/merge.cc: New
test.

    (cherry picked from commit 52ebc2be0990d6d3a46bb716164f9cef6f661021)

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:93d720f0ec7651e6a402dda33553817c1ad14ded

commit r10-10559-g93d720f0ec7651e6a402dda33553817c1ad14ded
Author: Pavel I. Kryukov <pavel.kryukov@phystech.edu>
Date:   Thu Jan 6 12:32:36 2022 +0000

    libstdc++: Add self-merge check to std::forward_list::merge [PR103853]

    This implements the proposed resolution of LWG 3088, so that x.merge(x)
    is a no-op, consistent with std::list::merge.

    Signed-off-by: Pavel I. Kryukov <pavel.kryukov@phystech.edu>

    Co-authored-by: Jonathan Wakely <jwakely@redhat.com>

    libstdc++-v3/ChangeLog:

            PR libstdc++/103853
            * include/bits/forward_list.tcc (forward_list::merge): Check for
            self-merge.
            * testsuite/23_containers/forward_list/operations/merge.cc: New
test.

    (cherry picked from commit 52ebc2be0990d6d3a46bb716164f9cef6f661021)

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:79b7b824346d3cb3101d1896f81ed16392b7471c

commit r9-10056-g79b7b824346d3cb3101d1896f81ed16392b7471c
Author: Pavel I. Kryukov <pavel.kryukov@phystech.edu>
Date:   Thu Jan 6 12:32:36 2022 +0000

    libstdc++: Add self-merge check to std::forward_list::merge [PR103853]

    This implements the proposed resolution of LWG 3088, so that x.merge(x)
    is a no-op, consistent with std::list::merge.

    Signed-off-by: Pavel I. Kryukov <pavel.kryukov@phystech.edu>

    Co-authored-by: Jonathan Wakely <jwakely@redhat.com>

    libstdc++-v3/ChangeLog:

            PR libstdc++/103853
            * include/bits/forward_list.tcc (forward_list::merge): Check for
            self-merge.
            * testsuite/23_containers/forward_list/operations/merge.cc: New
test.

    (cherry picked from commit 52ebc2be0990d6d3a46bb716164f9cef6f661021)

[Bug libstdc++/103853] std::forward_list::merge should check if __list != this

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103853

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |9.5
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for 9.5, 10.4, 11.4 and 12.1