[Bug libstdc++/101542] __gnu_cxx::sequence_buffer const copy constructor is badly broken
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101542 Jonathan Wakely changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED Target Milestone|--- |12.0 --- Comment #5 from Jonathan Wakely --- Should be fixed now.
[Bug libstdc++/101542] __gnu_cxx::sequence_buffer const copy constructor is badly broken
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101542 --- Comment #4 from CVS Commits --- The master branch has been updated by Jonathan Wakely : https://gcc.gnu.org/g:8edb61420502c62fa2cccdd98876a9aa039b72a6 commit r12-2437-g8edb61420502c62fa2cccdd98876a9aa039b72a6 Author: Jonathan Wakely Date: Wed Jul 21 15:29:19 2021 +0100 libstdc++: Make __gnu_cxx::sequence_buffer move-aware [PR101542] The PR explains that Clang trunk now selects a different constructor when a non-const sequence_buffer is returned in a context where it qualifies as an implicitly-movable entity. Because lookup is first performed using an rvalue, the sequence_buffer(const sequence_buffer&) constructor gets chosen, which makes a copy instead of a "pseudo-move" via the sequence_buffer(sequence_buffer&) constructor. The problem isn't seen with GCC because as noted in the r11-2412 commit log, GCC actually implements a slightly modified rule that avoids breaking exactly this type of code. This patch adds a move constructor to sequence_buffer, so that implicit or explicit moves will have the same effect, calling the sequence_buffer(sequence_buffer&) constructor. A move assignment operator is also added to make move assignment work similarly. Signed-off-by: Jonathan Wakely libstdc++-v3/ChangeLog: PR libstdc++/101542 * include/ext/rope (sequence_buffer): Add move constructor and move assignment operator. * testsuite/ext/rope/101542.cc: New test.
[Bug libstdc++/101542] __gnu_cxx::sequence_buffer const copy constructor is badly broken
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101542
--- Comment #3 from Jonathan Wakely ---
(In reply to Brooks Moses from comment #0)
> This started failing with a recent Clang change
> (https://github.com/llvm/llvm-project/commit/
> 7d2d5a3a6d7aaa40468c30250bf6b0938ef02c08), described as "Apply P1825 as
> Defect Report from C++11 up to C++20". See
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1825r0.html for the
> defect report details. I would guess that GCC will be applying a similar
> change.
GCC 11 already implemented that, see PR 91427 and the commit
https://gcc.gnu.org/g:1722e2013f05f1f1f99379dbaa0c0df356da731f
The for that commit says:
Discussion on the CWG reflector about how to avoid breaking the PR91212
test
in the new model settled on the model of doing only a single overload
resolution, with the variable treated as an xvalue that can bind to
non-const lvalue references. So this patch implements that approach. The
implementation does not use the existing LOOKUP_PREFER_RVALUE flag, but
instead sets a flag on the representation of the static_cast turning the
variable into an xvalue.
which says that calling sequence_buffer(sequence_buffer&) here is intended,
which is why we didn't see any change in the ext/rope/4.cc test when GCC
implemented it.
I still think we want to make sequence_buffer move-aware, so that you get the
same behaviour for:
template T f(T x) { return x; }
template T g(T x) { return std::move(x); }
when passed a sequence_buffer.
[Bug libstdc++/101542] __gnu_cxx::sequence_buffer const copy constructor is badly broken
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101542
--- Comment #2 from Jonathan Wakely ---
This should work:
--- a/libstdc++-v3/include/ext/rope
+++ b/libstdc++-v3/include/ext/rope
@@ -203,6 +203,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
std::copy(__x._M_buffer, __x._M_buffer + __x._M_buf_count, _M_buffer);
}
+ // Non-const "copy" modifies the parameter - yuck
sequence_buffer(sequence_buffer& __x)
{
__x.flush();
@@ -213,6 +214,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
sequence_buffer(_Sequence& __s)
: _M_prefix(&__s), _M_buf_count(0) { }
+ // Non-const "copy" modifies the parameter - yuck
sequence_buffer&
operator=(sequence_buffer& __x)
{
@@ -230,7 +232,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
std::copy(__x._M_buffer, __x._M_buffer + __x._M_buf_count, _M_buffer);
return *this;
}
-
+
+#if __cplusplus >= 201103L
+ sequence_buffer(sequence_buffer&& __x) : sequence_buffer(__x) { }
+ sequence_buffer& operator=(sequence_buffer&& __x) { return *this = __x;
}
+#endif
+
void
push_back(value_type __x)
{
[Bug libstdc++/101542] __gnu_cxx::sequence_buffer const copy constructor is badly broken
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101542 Jonathan Wakely changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2021-07-21 Ever confirmed|0 |1 --- Comment #1 from Jonathan Wakely --- (In reply to Brooks Moses from comment #0) > But sequence_buffer is more broken than that: it also has another > constructor, sequence_buffer(sequence_buffer&) that has different semantics: > it flushes the source first. So if you only ever use non-const > sequence_buffer objects, never modify a copied-from object, and never do > anything that would call the sequence_buffer(const sequence_buffer&) > constructor, it will appear to work. And that's what this test was relying > on. This constructor is similar to how std::auto_ptr "worked" and it was the source of numerous defect reports and problems. It was removed from the standard years ago and replaced with something that worked. > we don't have any code > that uses __gnu_cxx::sequence_buffer or __gnu_cxx::rope I doubt anybody does. It's ancient and not really maintained now. We can probably make it move-aware though.
