This is eventually what I did to only fix the PR.
In addition to what was initially done I've added the code to avoid
generating a fully usable iterator from a moved iterator instance.
libstdc++: Leave safe iterators in valid state post-move [PR127540]
Currently our safe iterators for debug mode fail to meet the
requirement
that they have a "valid but unspecified" state after being moved from.
They have a singular and non-copyable state, which means they cannot be
copied or passed around by value. This is non-conforming.
Define the moved-from state for our safe iterators to be the same as
their value-initialized state. This means they are copyable after being
moved from.
The downside of this change is that we no longer diagnose equality
comparison with a moved-from iterator, which is undefined behaviour.
Because value-initialized iterators can be compared, our moved-from
iterators are now also comparable. This seems like an acceptable
compromise in order to support copying them (which needs to work).
If we care about diagnosing invalid comparisons, we would need to
introduce a new moved-from state to the iterators. That could then be
checked in the iterator's constructors and assignment operators so that
moved-from iterators are allowed as the source of those operations. The
new state would not be checked in comparisons so that using them would
abort, as was the case before this commit, e.g.
/home/jwakely/gcc/17/include/c++/17.0.0/debug/safe_iterator.h:546:
In function:
constexpr bool gnu_debug::operator==(const
_Safe_iterator<gnu_cxx::
normal_iterator<int*, std::vector<int, std::allocator<int> >
>, std::
debug::vector<int>, std::forward_iterator_tag>::_Self&, const
_Safe_iterator<gnu_cxx::normal_iterator<int*, std::vector<int,
std::allocator<int> > >, std::debug::vector<int>,
std::forward_iterator_tag>::_Self&)
Error: attempt to compare a singular iterator to a
dereferenceable iterator.
Due to this change we also review the copy/move semantic so that we
do not try to attach
an instance to the sequence pointed-to by the singular, but
value-initialized, source iterator.
In such a situation the resulting instance will be also
value-initialized with the limitation
of such a state meaning comparable but nothing more.
libstdc++-v3/ChangeLog:
PR libstdc++/127540
* include/debug/safe_iterator.h (_Safe_iterator): Set
_M_version
to zero after move.
* include/debug/safe_local_iterator.h
(_Safe_local_iterator): Set _M_version
to zero after move.
* testsuite/24_iterators/debug/safe_iterator.cc: New test.
* testsuite/24_iterators/debug/safe_local_iterator.cc: New
test.
Co-authored-by: Jonathan Wakely <[email protected]>
Testing in progress, so far so good on new test cases in 24_iterators
and on 23_container/vector/* in _GLIBCXX_DEBUG mode.
Ok to commit if all tests good ?
François
On 10/16/26 12:16, Jonathan Wakely wrote:
Currently our safe iterators for debug mode fail to meet the requirement
that they have a "valid but unspecified" state after being moved from.
They have a singular and non-copyable state, which means they cannot be
copied or passed around by value. This is non-conforming.
Define the moved-from state for our safe iterators to be the same as
their value-initialized state. This means they are copyable after being
moved from.
The downside of this change is that we no longer diagnose equality
comparison with a moved-from iterator, which is undefined behaviour.
Because value-initialized iterators can be compared, our moved-from
iterators are now also comparable. This seems like an acceptable
compromise in order to support copying them (which needs to work).
If we care about diagnosing invalid comparisons, we would need to
introduce a new moved-from state to the iterators. That could then be
checked in the iterator's constructors and assignment operators so that
moved-from iterators are allowed as the source of those operations. The
new state would not be checked in comparisons so that using them would
abort, as was the case before this commit, e.g.
/home/jwakely/gcc/17/include/c++/17.0.0/debug/safe_iterator.h:546:
In function:
constexpr bool gnu_debug::operator==(const _Safe_iterator<gnu_cxx::
normal_iterator<int*, std::vector<int, std::allocator<int> > >, std::
debug::vector<int>, std::forward_iterator_tag>::_Self&, const
_Safe_iterator<gnu_cxx::normal_iterator<int*, std::vector<int,
std::allocator<int> > >, std::debug::vector<int>,
std::forward_iterator_tag>::_Self&)
Error: attempt to compare a singular iterator to a dereferenceable iterator.
libstdc++-v3/ChangeLog:
PR libstdc++/127540
* include/debug/safe_iterator.h (_Safe_iterator): Set _M_version
to zero after move.
* testsuite/24_iterators/debug/127540.cc: New test.
---
Tested x86_64-linux.
Posting this as an RFC because it's not the only solution.
In bugzilla François suggested an alternative solution that would leave
moved-from iterators in the same state they are now (singular and
without clearing _M_version) but copying them would produce a
value-initialized iterator. I haven't tested that, but I think it would
mean moved-from iterators would abort if used in equality comparisons
(but copies of them would not). That might be preferable, although the
"moved-from means value-initialized" model is simpler to explain.
libstdc++-v3/include/debug/safe_iterator.h | 7 +--
.../testsuite/24_iterators/debug/127540.cc | 54 +++++++++++++++++++ 2
files changed, 56 insertions(+), 5 deletions(-) create mode 100644
libstdc++-v3/testsuite/24_iterators/debug/127540.cc diff --git
a/libstdc++-v3/include/debug/safe_iterator.h
b/libstdc++-v3/include/debug/safe_iterator.h index
8c049961cca1..6a5b8fd61fca 100644 ---
a/libstdc++-v3/include/debug/safe_iterator.h +++
b/libstdc++-v3/include/debug/safe_iterator.h @@ -213,15 +213,11 @@
namespace __gnu_debug return; } -
_GLIBCXX_DEBUG_VERIFY(!__x._M_singular() - ||
__x._M_value_initialized(), - _M_message(__msg_init_copy_singular) -
._M_iterator(*this, "this")
- ._M_iterator(__x, "other")); const _Safe_sequence_base* __seq = __x._M_sequence;
__x._M_detach(); std::swap(base(), __x.base()); _M_attach(__seq); +
__x._M_version = 0; } #endif @@ -328,6 +324,7 @@ namespace __gnu_debug
__x._M_detach(); __x.base() = _Iterator(); + __x._M_version = 0;
return *this; } #endif diff --git
a/libstdc++-v3/testsuite/24_iterators/debug/127540.cc
b/libstdc++-v3/testsuite/24_iterators/debug/127540.cc new file mode
100644 index 000000000000..79c6bab67d02 --- /dev/null +++
b/libstdc++-v3/testsuite/24_iterators/debug/127540.cc @@ -0,0 +1,54 @@
+// { dg-options "-D_GLIBCXX_DEBUG" }
+// { dg-do run { target c++11 } }
+
+#include <vector>
+#include <algorithm>
+
+static void
+test_swap()
+{
+ std::vector<int> v{1};
+ auto i = v.begin();
+ std::swap(i, i);
+}
+
+static void
+test_copy()
+{
+ std::vector<int> v{1};
+ auto i = v.begin();
+ auto j = std::move(i); // i enters moved-from state via move constructor
+ j = i; // copy assignment using moved-from iterator
+ auto k = i; // copy construction using moved-from iterator
+ k = i; // copy assignment using moved-from iterator
+
+ i = v.begin();
+ j = std::move(i); // i enters moved-from state via move assignment
+ j = i; // copy assignment using moved-from iterator
+ auto l = i; // copy construction using moved-from iterator
+ l = i; // copy assignment using moved-from iterator
+}
+
+static void
+test_move()
+{
+ std::vector<int> v{1};
+ auto i = v.begin();
+ auto j = std::move(i); // i enters moved-from state via move constructor
+ j = std::move(i); // move assignment using moved-from iterator
+ auto k = std::move(i); // move construction using moved-from iterator
+ k = std::move(i); // move assignment using moved-from iterator
+
+ i = v.begin();
+ j = std::move(i); // i enters moved-from state via move assignment
+ j = std::move(i); // move assignment using moved-from iterator
+ auto l = std::move(i); // move construction using moved-from iterator
+ l = std::move(i); // move assignment using moved-from iterator
+}
+
+int main()
+{
+ test_swap();
+ test_copy();
+ test_move();
+}
diff --git a/libstdc++-v3/include/debug/safe_iterator.h
b/libstdc++-v3/include/debug/safe_iterator.h
index 8c049961cca..04e1b7029d8 100644
--- a/libstdc++-v3/include/debug/safe_iterator.h
+++ b/libstdc++-v3/include/debug/safe_iterator.h
@@ -195,7 +195,13 @@ namespace __gnu_debug
_M_message(__msg_init_copy_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
+ if (!__x._M_singular())
_M_attach(__x._M_sequence);
+ else
+ {
+ _M_version = __x._M_version;
+ _M_sequence = __x._M_sequence;
+ }
}
#if __cplusplus >= 201103L
@@ -218,11 +224,22 @@ namespace __gnu_debug
_M_message(__msg_init_copy_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
+
+ if (!__x._M_singular())
+ {
const _Safe_sequence_base* __seq = __x._M_sequence;
__x._M_detach();
std::swap(base(), __x.base());
+ __x._M_version = 0;
_M_attach(__seq);
}
+ else
+ {
+ base() = __x.base();
+ _M_version = __x._M_version;
+ _M_sequence = __x._M_sequence;
+ }
+ }
#endif
/**
@@ -249,7 +266,13 @@ namespace __gnu_debug
_M_message(__msg_init_const_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
+ if (!__x._M_singular())
_M_attach(__x._M_sequence);
+ else
+ {
+ _M_version = __x._M_version;
+ _M_sequence = __x._M_sequence;
+ }
}
/**
@@ -273,11 +296,20 @@ namespace __gnu_debug
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
+ if (__x._M_singular())
+ {
+ _M_detach();
+ base() = __x.base();
+ _M_version = __x._M_version;
+ _M_sequence = __x._M_sequence;
+ return *this;
+ }
+
if (this->_M_sequence && this->_M_sequence == __x._M_sequence)
_GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN {
__gnu_cxx::__scoped_lock __l(this->_M_get_mutex());
base() = __x.base();
- _M_version = __x._M_sequence->_M_version;
+ _M_version = __x._M_version;
} _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END
else
{
@@ -313,11 +345,20 @@ namespace __gnu_debug
if (std::__addressof(__x) == this)
return *this;
+ if (__x._M_singular())
+ {
+ _M_detach();
+ base() = __x.base();
+ _M_version = __x._M_version;
+ _M_sequence = __x._M_sequence;
+ return *this;
+ }
+
if (this->_M_sequence && this->_M_sequence == __x._M_sequence)
_GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN {
__gnu_cxx::__scoped_lock __l(this->_M_get_mutex());
base() = __x.base();
- _M_version = __x._M_sequence->_M_version;
+ _M_version = __x._M_version;
} _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END
else
{
@@ -328,6 +369,7 @@ namespace __gnu_debug
__x._M_detach();
__x.base() = _Iterator();
+ __x._M_version = 0;
return *this;
}
#endif
diff --git a/libstdc++-v3/include/debug/safe_local_iterator.h
b/libstdc++-v3/include/debug/safe_local_iterator.h
index 18b7b0b6191..16b8ce90d94 100644
--- a/libstdc++-v3/include/debug/safe_local_iterator.h
+++ b/libstdc++-v3/include/debug/safe_local_iterator.h
@@ -122,7 +122,13 @@ namespace __gnu_debug
_M_message(__msg_init_copy_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
+ if (!__x._M_singular())
_M_attach(__x._M_safe_container());
+ else
+ {
+ _M_version = __x._M_version;
+ _M_sequence = __x._M_sequence;
+ }
}
/**
@@ -137,11 +143,22 @@ namespace __gnu_debug
_M_message(__msg_init_copy_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
+
+ if (!__x._M_singular())
+ {
auto __cont = __x._M_safe_container();
__x._M_detach();
std::swap(base(), __x.base());
+ __x._M_version = 0;
_M_attach(__cont);
}
+ else
+ {
+ base() = __x.base();
+ _M_version = __x._M_version;
+ _M_sequence = __x._M_sequence;
+ }
+ }
/**
* @brief Converting constructor from a mutable iterator to a
@@ -162,7 +179,13 @@ namespace __gnu_debug
_M_message(__msg_init_const_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
+ if (!__x._M_singular())
_M_attach(__x._M_safe_container());
+ else
+ {
+ _M_version = __x._M_version;
+ _M_sequence = __x._M_sequence;
+ }
}
/**
@@ -179,6 +202,15 @@ namespace __gnu_debug
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
+ if (__x._M_singular())
+ {
+ _M_detach();
+ base() = __x.base();
+ _M_version = __x._M_version;
+ _M_sequence = __x._M_sequence;
+ return *this;
+ }
+
if (this->_M_sequence && this->_M_sequence == __x._M_sequence)
{
__gnu_cxx::__scoped_lock __l(this->_M_get_mutex());
@@ -211,11 +243,20 @@ namespace __gnu_debug
if (std::__addressof(__x) == this)
return *this;
+ if (__x._M_singular())
+ {
+ _M_detach();
+ base() = __x.base();
+ _M_version = __x._M_version;
+ _M_sequence = __x._M_sequence;
+ return *this;
+ }
+
if (this->_M_sequence && this->_M_sequence == __x._M_sequence)
{
__gnu_cxx::__scoped_lock __l(this->_M_get_mutex());
base() = __x.base();
- _M_version = __x._M_sequence->_M_version;
+ _M_version = __x._M_version;
}
else
{
@@ -226,6 +267,7 @@ namespace __gnu_debug
__x._M_detach();
__x.base() = _Iterator();
+ __x._M_version = 0;
return *this;
}
diff --git a/libstdc++-v3/testsuite/24_iterators/debug/safe_iterator.cc
b/libstdc++-v3/testsuite/24_iterators/debug/safe_iterator.cc
new file mode 100644
index 00000000000..eabc70f239f
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/debug/safe_iterator.cc
@@ -0,0 +1,104 @@
+// { dg-options "-D_GLIBCXX_DEBUG" }
+// { dg-do run { target c++11 } }
+
+#include <vector>
+#include <algorithm>
+
+#include <testsuite_hooks.h>
+
+static void
+test_swap()
+{
+ std::vector<int> v{1};
+ auto i = v.begin();
+ std::swap(i, i);
+ VERIFY( *i == 1 );
+}
+
+static void
+test_copy()
+{
+ std::vector<int> v{1};
+ auto i = v.begin();
+ auto j = std::move(i); // i enters moved-from state via move constructor
+ VERIFY( *j == 1 );
+ j = i; // copy assignment using moved-from iterator
+ VERIFY( j == i );
+ auto k = i; // copy construction using moved-from iterator
+ VERIFY( k == i );
+ k = i; // copy assignment using moved-from iterator
+ VERIFY( k == i );
+
+ i = v.begin();
+ j = std::move(i); // i enters moved-from state via move assignment
+ VERIFY( *j == 1 );
+ j = i; // copy assignment using moved-from iterator
+ VERIFY( j == i );
+ auto l = i; // copy construction using moved-from iterator
+ VERIFY( l == i );
+ l = i; // copy assignment using moved-from iterator
+ VERIFY( l == i );
+
+ i = v.begin();
+ j = v.begin();
+ j = std::move(i); // i enters moved-from state via move constructor
+ VERIFY( *j == 1 );
+}
+
+static void
+test_move()
+{
+ std::vector<int> v{1};
+ auto i = v.begin();
+ auto j = std::move(i); // i enters moved-from state via move constructor
+ VERIFY( *j == 1 );
+ j = std::move(i); // move assignment using moved-from iterator
+ VERIFY( j == i );
+ auto k = std::move(i); // move construction using moved-from iterator
+ VERIFY( k == i );
+ k = std::move(i); // move assignment using moved-from iterator
+ VERIFY( k == i );
+
+ i = v.begin();
+ j = std::move(i); // i enters moved-from state via move assignment
+ VERIFY( *j == 1 );
+ j = std::move(i); // move assignment using moved-from iterator
+ VERIFY( j == i );
+ auto l = std::move(i); // move construction using moved-from iterator
+ VERIFY( l == i );
+ l = std::move(i); // move assignment using moved-from iterator
+ VERIFY( l == i );
+}
+
+static void
+test_conversion()
+{
+ std::vector<int> v{1};
+ auto i = v.begin();
+ typename std::vector<int>::const_iterator j = std::move(i); // i enters
moved-from state via move constructor
+ VERIFY( *j == 1 );
+ j = std::move(i); // move assignment using moved-from iterator
+ VERIFY( j == i );
+ typename std::vector<int>::const_iterator k = std::move(i); // move
construction using moved-from iterator
+ VERIFY( k == i );
+ k = std::move(i); // move assignment using moved-from iterator
+ VERIFY( k == i );
+
+ i = v.begin();
+ j = std::move(i); // i enters moved-from state via move assignment
+ VERIFY( *j == 1 );
+ j = std::move(i); // move assignment using moved-from iterator
+ VERIFY( j == i );
+ typename std::vector<int>::const_iterator l = std::move(i); // move
construction using moved-from iterator
+ VERIFY( l == i );
+ l = std::move(i); // move assignment using moved-from iterator
+ VERIFY( l == i );
+}
+
+int main()
+{
+ test_swap();
+ test_copy();
+ test_move();
+ test_conversion();
+}
diff --git a/libstdc++-v3/testsuite/24_iterators/debug/safe_local_iterator.cc
b/libstdc++-v3/testsuite/24_iterators/debug/safe_local_iterator.cc
new file mode 100644
index 00000000000..af69ddb2b40
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/debug/safe_local_iterator.cc
@@ -0,0 +1,104 @@
+// { dg-options "-D_GLIBCXX_DEBUG" }
+// { dg-do run { target c++11 } }
+
+#include <unordered_map>
+#include <algorithm>
+
+#include <testsuite_hooks.h>
+
+static void
+test_swap()
+{
+ std::unordered_map<int, int> um{ { 1, 1 } };
+ auto i = um.begin(1);
+ std::swap(i, i);
+ VERIFY( i->first == 1 );
+}
+
+static void
+test_copy()
+{
+ std::unordered_map<int, int> um{ { 1, 1 } };
+ auto i = um.begin(1);
+ auto j = std::move(i); // i enters moved-from state via move constructor
+ VERIFY( j->first == 1 );
+ j = i; // copy assignment using moved-from iterator
+ VERIFY( j == i );
+ auto k = i; // copy construction using moved-from iterator
+ VERIFY( k == i );
+ k = i; // copy assignment using moved-from iterator
+ VERIFY( k == i );
+
+ i = um.begin(1);
+ j = std::move(i); // i enters moved-from state via move assignment
+ VERIFY( j->first == 1 );
+ j = i; // copy assignment using moved-from iterator
+ VERIFY( j == i );
+ auto l = i; // copy construction using moved-from iterator
+ VERIFY( l == i );
+ l = i; // copy assignment using moved-from iterator
+ VERIFY( l == i );
+
+ i = um.begin(1);
+ j = um.begin(1);
+ j = std::move(i); // i enters moved-from state via move constructor
+ VERIFY( j->first == 1 );
+}
+
+static void
+test_move()
+{
+ std::unordered_map<int, int> um{ { 1, 1 } };
+ auto i = um.begin(1);
+ auto j = std::move(i); // i enters moved-from state via move constructor
+ VERIFY( j->first == 1 );
+ j = std::move(i); // move assignment using moved-from iterator
+ VERIFY( j == i );
+ auto k = std::move(i); // move construction using moved-from iterator
+ VERIFY( k == i );
+ k = std::move(i); // move assignment using moved-from iterator
+ VERIFY( k == i );
+
+ i = um.begin(1);
+ j = std::move(i); // i enters moved-from state via move assignment
+ VERIFY( j->first == 1 );
+ j = std::move(i); // move assignment using moved-from iterator
+ VERIFY( j == i );
+ auto l = std::move(i); // move construction using moved-from iterator
+ VERIFY( l == i );
+ l = std::move(i); // move assignment using moved-from iterator
+ VERIFY( l == i );
+}
+
+static void
+test_conversion()
+{
+ std::unordered_map<int, int> um{ { 1, 1 } };
+ auto i = um.begin(1);
+ typename std::unordered_map<int, int>::const_local_iterator j =
std::move(i); // i enters moved-from state via move constructor
+ VERIFY( j->first == 1 );
+ j = std::move(i); // move assignment using moved-from iterator
+ VERIFY( j == i );
+ typename std::unordered_map<int, int>::const_local_iterator k =
std::move(i); // move construction using moved-from iterator
+ VERIFY( k == i );
+ k = std::move(i); // move assignment using moved-from iterator
+ VERIFY( k == i );
+
+ i = um.begin(1);
+ j = std::move(i); // i enters moved-from state via move assignment
+ VERIFY( j->first == 1 );
+ j = std::move(i); // move assignment using moved-from iterator
+ VERIFY( j == i );
+ typename std::unordered_map<int, int>::const_local_iterator l =
std::move(i); // move construction using moved-from iterator
+ VERIFY( l == i );
+ l = std::move(i); // move assignment using moved-from iterator
+ VERIFY( l == i );
+}
+
+int main()
+{
+ test_swap();
+ test_copy();
+ test_move();
+ test_conversion();
+}
