public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug libstdc++/101542] New: __gnu_cxx::sequence_buffer const copy constructor is badly broken
@ 2021-07-21 2:51 brooks at gcc dot gnu.org
2021-07-21 10:00 ` [Bug libstdc++/101542] " redi at gcc dot gnu.org
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: brooks at gcc dot gnu.org @ 2021-07-21 2:51 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101542
Bug ID: 101542
Summary: __gnu_cxx::sequence_buffer const copy constructor is
badly broken
Product: gcc
Version: unknown
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: libstdc++
Assignee: unassigned at gcc dot gnu.org
Reporter: brooks at gcc dot gnu.org
Target Milestone: ---
This problem showed up when we were running the GCC 4.9.4 testsuite with the
latest Clang trunk as the compiler, and found that the ext/rope/4.cc test
started failing -- producing "wibblewibble" when it should produce "wibble".
However, the problem is broader than that, and affects much more current GCC
versions as well -- as can be seen at https://godbolt.org/z/74W78rdWP.
Compiling the following program with Clang trunk produces "hellohello" as
output:
----
#include <ext/rope>
#include <iostream>
template<typename T> T make_copy(const T &x) { return x; }
int main() {
std::string s;
__gnu_cxx::sequence_buffer<std::string> a(s);
{
__gnu_cxx::sequence_buffer<std::string> b = a;
b.push_back('h');
b.push_back('e');
b.push_back('l');
b.push_back('l');
b.push_back('o');
// Making a copy causes sequence_buffer to break.
make_copy(b);
}
std::cout << s;
}
----
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.
My colleagues Jorge Gorbe and Richard Smith did a lot of digging on this, and
Richard explains the problem as follows:
----
The problem with sequence_buffer is that the sequence_buffer(const
sequence_buffer&) copy constructor copies the pending characters in the buffer
to the new copy, and when both are destroyed, they get flushed to the
underlying sequence twice.
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.
Part of the consequence of the Clang change is that in code like this:
sequence_buffer f(sequence_buffer x) {
return x;
}
... the return statement unconditionally behaves like return
(sequence_buffer&&)x;. And that means that we now choose the
sequence_buffer(const sequence_buffer&) constructor, not the
sequence_buffer(sequence_buffer&) constructor, because a non-const lvalue
reference can't bind to an rvalue. Ultimately, I think that happens at line
342 of stl_algobase.h ("return __result"), here:
https://gcc.gnu.org/git?p=gcc.git;a=blame;f=libstdc%2B%2B-v3/include/bits/stl_algobase.h;hb=9b61c47826156fe17fd5f4306470ade01e2fc4dc#l238.
So: this libstdc++ code is broken, and the test really should never have
passed.
----
In our case, we're simply xfailing the test since we don't have any code that
uses __gnu_cxx::sequence_buffer or __gnu_cxx::rope, but I'm passing this bug
report along in hopes that it's useful to the libstdc++ maintainers. :)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Bug libstdc++/101542] __gnu_cxx::sequence_buffer const copy constructor is badly broken
2021-07-21 2:51 [Bug libstdc++/101542] New: __gnu_cxx::sequence_buffer const copy constructor is badly broken brooks at gcc dot gnu.org
@ 2021-07-21 10:00 ` redi at gcc dot gnu.org
2021-07-21 12:46 ` redi at gcc dot gnu.org
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: redi at gcc dot gnu.org @ 2021-07-21 10:00 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101542
Jonathan Wakely <redi at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|UNCONFIRMED |NEW
Last reconfirmed| |2021-07-21
Ever confirmed|0 |1
--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Bug libstdc++/101542] __gnu_cxx::sequence_buffer const copy constructor is badly broken
2021-07-21 2:51 [Bug libstdc++/101542] New: __gnu_cxx::sequence_buffer const copy constructor is badly broken brooks at gcc dot gnu.org
2021-07-21 10:00 ` [Bug libstdc++/101542] " redi at gcc dot gnu.org
@ 2021-07-21 12:46 ` redi at gcc dot gnu.org
2021-07-21 14:24 ` redi at gcc dot gnu.org
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: redi at gcc dot gnu.org @ 2021-07-21 12:46 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101542
--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
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)
{
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Bug libstdc++/101542] __gnu_cxx::sequence_buffer const copy constructor is badly broken
2021-07-21 2:51 [Bug libstdc++/101542] New: __gnu_cxx::sequence_buffer const copy constructor is badly broken brooks at gcc dot gnu.org
2021-07-21 10:00 ` [Bug libstdc++/101542] " redi at gcc dot gnu.org
2021-07-21 12:46 ` redi at gcc dot gnu.org
@ 2021-07-21 14:24 ` redi at gcc dot gnu.org
2021-07-21 16:20 ` cvs-commit at gcc dot gnu.org
2021-07-21 16:28 ` redi at gcc dot gnu.org
4 siblings, 0 replies; 6+ messages in thread
From: redi at gcc dot gnu.org @ 2021-07-21 14:24 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101542
--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(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<typename T> T f(T x) { return x; }
template<typename T> T g(T x) { return std::move(x); }
when passed a sequence_buffer.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Bug libstdc++/101542] __gnu_cxx::sequence_buffer const copy constructor is badly broken
2021-07-21 2:51 [Bug libstdc++/101542] New: __gnu_cxx::sequence_buffer const copy constructor is badly broken brooks at gcc dot gnu.org
` (2 preceding siblings ...)
2021-07-21 14:24 ` redi at gcc dot gnu.org
@ 2021-07-21 16:20 ` cvs-commit at gcc dot gnu.org
2021-07-21 16:28 ` redi at gcc dot gnu.org
4 siblings, 0 replies; 6+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2021-07-21 16:20 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101542
--- Comment #4 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:8edb61420502c62fa2cccdd98876a9aa039b72a6
commit r12-2437-g8edb61420502c62fa2cccdd98876a9aa039b72a6
Author: Jonathan Wakely <jwakely@redhat.com>
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 <jwakely@redhat.com>
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Bug libstdc++/101542] __gnu_cxx::sequence_buffer const copy constructor is badly broken
2021-07-21 2:51 [Bug libstdc++/101542] New: __gnu_cxx::sequence_buffer const copy constructor is badly broken brooks at gcc dot gnu.org
` (3 preceding siblings ...)
2021-07-21 16:20 ` cvs-commit at gcc dot gnu.org
@ 2021-07-21 16:28 ` redi at gcc dot gnu.org
4 siblings, 0 replies; 6+ messages in thread
From: redi at gcc dot gnu.org @ 2021-07-21 16:28 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101542
Jonathan Wakely <redi at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
Target Milestone|--- |12.0
--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Should be fixed now.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-21 16:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21 2:51 [Bug libstdc++/101542] New: __gnu_cxx::sequence_buffer const copy constructor is badly broken brooks at gcc dot gnu.org
2021-07-21 10:00 ` [Bug libstdc++/101542] " redi at gcc dot gnu.org
2021-07-21 12:46 ` redi at gcc dot gnu.org
2021-07-21 14:24 ` redi at gcc dot gnu.org
2021-07-21 16:20 ` cvs-commit at gcc dot gnu.org
2021-07-21 16:28 ` redi at gcc dot gnu.org
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).