Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[François Dumont]

Hi

This is what I started to do.

For now I haven't touch to __cpp_lib_null_iterators definition as 
_Safe_local_iterator still need some work.

libstdc++: Implement N3644 on _Safe_iterator<> [PR114316]

Consider range of value-initialized iterators as valid and empty.

libstdc++-v3/ChangeLog:

         PR libstdc++/114316
         * include/debug/safe_iterator.tcc 
(_Safe_iterator<>::_M_valid_range):
         First check if both iterators are value-initialized before 
checking if
         singular.
         * testsuite/23_containers/set/debug/114316.cc: New test case.
         * testsuite/23_containers/vector/debug/114316.cc: New test case.

Tested under Linux x86_64, ok to commit ?

François


On 12/03/2024 10:52, Jonathan Wakely wrote:
> On Tue, 12 Mar 2024 at 01:03, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On Tue, 12 Mar 2024 at 00:55, Maciej Miera <maciej.miera@gmail.com> wrote:
>>>
>>>
>>> Wiadomość napisana przez Jonathan Wakely <jwakely@redhat.com> w dniu 11.03.2024, o godz. 21:40:
>>>
>>> On Mon, 11 Mar 2024 at 20:07, Maciej Miera <maciej.miera@gmail.com> wrote:
>>>
>>>
>>> Hello,
>>>
>>> I have tried to introduce an extra level of safety to my codebase and utilize _GLIBCXX_DEBUG in my test builds in order to catch faulty iterators.
>>> However, I have encountered the following problem: I would like to utilize singular, value-initialized iterators as an arbitrary "null range”.
>>> However, this leads to failed assertions in std:: algorithms taking such range.
>>>
>>> Consider the following code sample with find_if:
>>>
>>> #include <map>
>>> #include <algorithm>
>>> #include <iterator>
>>>
>>> #ifndef __cpp_lib_null_iterators
>>> #warning "Not standard compliant"
>>> #endif
>>>
>>> int main()
>>> {
>>>     std::multimap<char, int>::iterator it1{};
>>>     std::multimap<char, int>::iterator it2{};
>>>
>>>     (void) (it1==it2); // OK
>>>     (void) std::find_if(
>>>         it1, it2, [](const auto& el) { return el.second == 8;});
>>> }
>>>
>>> Compiled with -std=c++20 and -D_GLIBCXX_DEBUG it produces the warning "Not standard compliant"
>>> and the execution results in the following assert failure:
>>>
>>> /opt/compiler-explorer/gcc-12.2.0/include/c++/12.2.0/bits/stl_algo.h:3875:
>>> In function:
>>>     constexpr _IIter std::find_if(_IIter, _IIter, _Predicate) [with _IIter =
>>>     gnu_debug::_Safe_iterator<_Rb_tree_iterator<pair<const char, int> >,
>>>     debug::multimap<char, int>, bidirectional_iterator_tag>; _Predicate =
>>>     main()::<lambda(const auto:16&)>]
>>>
>>> The question is though: is it by design, or is it just a mere oversight? The warning actually suggest the first option.
>>> If it is an intentional design choice, could you provide some rationale behind it, please?
>>>
>>>
>>> The macro was not defined because the C++14 rule wasn't implemented
>>> for debug mode, but that should have been fixed for GCC 11, according
>>> to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98466 and
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70303
>>> So we should be able to define macro now, except maybe it wasn't fixed
>>> for the RB tree containers.
>>>
>>>
>>>
>>> Just to make sure there are no misunderstandings: comparison via == works fine. The feature check macro without _GLIBCXX_DEBUG and with <iterator> included is also fine. Maybe the need to include a header is the issue, but that’s not the core of the problem anyway.
>> No, it has nothing to do with the headers included. The feature test
>> macro is defined like so:
>>
>> # if (__cplusplus >= 201402L) && (!defined(_GLIBCXX_DEBUG))
>> #  define __glibcxx_null_iterators 201304L
>>
>> It's a very deliberate choice to not define it when _GLIBCXX_DEBUG is
>> defined. But as I said, I think we should have changed that.
>>
>>> The actual question is though, whether passing singular iterators to std algorithms (like find_if) should make the asserts at the beginning of the algo function fail when compiled with _GLIBCXX_DEBUG. IMHO, intuitively it should not, as comparing iterators equal would just ensure early exit and return of the same singular iterator.
>>> This seems not to be the case though. The actual message is this:
>>> Error: the function requires a valid iterator range [first, last).
>>> What bothers me is whether the empty virtual range limited by two same singular iterators is actually valid or not.
>> Yes, it's valid. So the bug is in the __glibcxx_requires_valid_range macro.
> Thanks for the bugzilla report:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114316
> We'll get it fixed!
>

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 4963 bytes --]

With the patch, sorry.

On 14/03/2024 22:49, François Dumont wrote:
> Hi
>
> This is what I started to do.
>
> For now I haven't touch to __cpp_lib_null_iterators definition as 
> _Safe_local_iterator still need some work.
>
> libstdc++: Implement N3644 on _Safe_iterator<> [PR114316]
>
> Consider range of value-initialized iterators as valid and empty.
>
> libstdc++-v3/ChangeLog:
>
>         PR libstdc++/114316
>         * include/debug/safe_iterator.tcc 
> (_Safe_iterator<>::_M_valid_range):
>         First check if both iterators are value-initialized before 
> checking if
>         singular.
>         * testsuite/23_containers/set/debug/114316.cc: New test case.
>         * testsuite/23_containers/vector/debug/114316.cc: New test case.
>
> Tested under Linux x86_64, ok to commit ?
>
> François
>
>
> On 12/03/2024 10:52, Jonathan Wakely wrote:
>> On Tue, 12 Mar 2024 at 01:03, Jonathan Wakely <jwakely@redhat.com> 
>> wrote:
>>> On Tue, 12 Mar 2024 at 00:55, Maciej Miera <maciej.miera@gmail.com> 
>>> wrote:
>>>>
>>>>
>>>> Wiadomość napisana przez Jonathan Wakely <jwakely@redhat.com> w 
>>>> dniu 11.03.2024, o godz. 21:40:
>>>>
>>>> On Mon, 11 Mar 2024 at 20:07, Maciej Miera <maciej.miera@gmail.com> 
>>>> wrote:
>>>>
>>>>
>>>> Hello,
>>>>
>>>> I have tried to introduce an extra level of safety to my codebase 
>>>> and utilize _GLIBCXX_DEBUG in my test builds in order to catch 
>>>> faulty iterators.
>>>> However, I have encountered the following problem: I would like to 
>>>> utilize singular, value-initialized iterators as an arbitrary "null 
>>>> range”.
>>>> However, this leads to failed assertions in std:: algorithms taking 
>>>> such range.
>>>>
>>>> Consider the following code sample with find_if:
>>>>
>>>> #include <map>
>>>> #include <algorithm>
>>>> #include <iterator>
>>>>
>>>> #ifndef __cpp_lib_null_iterators
>>>> #warning "Not standard compliant"
>>>> #endif
>>>>
>>>> int main()
>>>> {
>>>>     std::multimap<char, int>::iterator it1{};
>>>>     std::multimap<char, int>::iterator it2{};
>>>>
>>>>     (void) (it1==it2); // OK
>>>>     (void) std::find_if(
>>>>         it1, it2, [](const auto& el) { return el.second == 8;});
>>>> }
>>>>
>>>> Compiled with -std=c++20 and -D_GLIBCXX_DEBUG it produces the 
>>>> warning "Not standard compliant"
>>>> and the execution results in the following assert failure:
>>>>
>>>> /opt/compiler-explorer/gcc-12.2.0/include/c++/12.2.0/bits/stl_algo.h:3875: 
>>>>
>>>> In function:
>>>>     constexpr _IIter std::find_if(_IIter, _IIter, _Predicate) [with 
>>>> _IIter =
>>>> gnu_debug::_Safe_iterator<_Rb_tree_iterator<pair<const char, int> >,
>>>>     debug::multimap<char, int>, bidirectional_iterator_tag>; 
>>>> _Predicate =
>>>>     main()::<lambda(const auto:16&)>]
>>>>
>>>> The question is though: is it by design, or is it just a mere 
>>>> oversight? The warning actually suggest the first option.
>>>> If it is an intentional design choice, could you provide some 
>>>> rationale behind it, please?
>>>>
>>>>
>>>> The macro was not defined because the C++14 rule wasn't implemented
>>>> for debug mode, but that should have been fixed for GCC 11, according
>>>> to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98466 and
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70303
>>>> So we should be able to define macro now, except maybe it wasn't fixed
>>>> for the RB tree containers.
>>>>
>>>>
>>>>
>>>> Just to make sure there are no misunderstandings: comparison via == 
>>>> works fine. The feature check macro without _GLIBCXX_DEBUG and with 
>>>> <iterator> included is also fine. Maybe the need to include a 
>>>> header is the issue, but that’s not the core of the problem anyway.
>>> No, it has nothing to do with the headers included. The feature test
>>> macro is defined like so:
>>>
>>> # if (__cplusplus >= 201402L) && (!defined(_GLIBCXX_DEBUG))
>>> #  define __glibcxx_null_iterators 201304L
>>>
>>> It's a very deliberate choice to not define it when _GLIBCXX_DEBUG is
>>> defined. But as I said, I think we should have changed that.
>>>
>>>> The actual question is though, whether passing singular iterators 
>>>> to std algorithms (like find_if) should make the asserts at the 
>>>> beginning of the algo function fail when compiled with 
>>>> _GLIBCXX_DEBUG. IMHO, intuitively it should not, as comparing 
>>>> iterators equal would just ensure early exit and return of the same 
>>>> singular iterator.
>>>> This seems not to be the case though. The actual message is this:
>>>> Error: the function requires a valid iterator range [first, last).
>>>> What bothers me is whether the empty virtual range limited by two 
>>>> same singular iterators is actually valid or not.
>>> Yes, it's valid. So the bug is in the __glibcxx_requires_valid_range 
>>> macro.
>> Thanks for the bugzilla report:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114316
>> We'll get it fixed!
>>

[-- Attachment #2: pr104316.txt --]
[-- Type: text/plain, Size: 2121 bytes --]

diff --git a/libstdc++-v3/include/debug/safe_iterator.tcc b/libstdc++-v3/include/debug/safe_iterator.tcc
index a8b24233e85..4b2baf2980e 100644
--- a/libstdc++-v3/include/debug/safe_iterator.tcc
+++ b/libstdc++-v3/include/debug/safe_iterator.tcc
@@ -194,6 +194,12 @@ namespace __gnu_debug
 		   std::pair<difference_type, _Distance_precision>& __dist,
 		   bool __check_dereferenceable) const
     {
+      if (_M_value_initialized() && __rhs._M_value_initialized())
+	{
+	  __dist = std::make_pair(0, __dp_exact);
+	  return true;
+	}
+
       if (_M_singular() || __rhs._M_singular() || !_M_can_compare(__rhs))
 	return false;
 
@@ -218,6 +224,12 @@ namespace __gnu_debug
 		   std::pair<difference_type,
 			     _Distance_precision>& __dist) const
     {
+      if (this->_M_value_initialized() && __rhs._M_value_initialized())
+	{
+	  __dist = std::make_pair(0, __dp_exact);
+	  return true;
+	}
+
       if (this->_M_singular() || __rhs._M_singular()
 	  || !this->_M_can_compare(__rhs))
 	return false;
diff --git a/libstdc++-v3/testsuite/23_containers/set/debug/114316.cc b/libstdc++-v3/testsuite/23_containers/set/debug/114316.cc
new file mode 100644
index 00000000000..126ec89b5e0
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/set/debug/114316.cc
@@ -0,0 +1,16 @@
+// { dg-do run { target c++11 } }
+// { dg-require-debug-mode "" }
+
+// PR libstdc++/114316
+
+#include <set>
+#include <algorithm>
+
+#include <testsuite_hooks.h>
+
+int main()
+{
+  std::set<int>::iterator it{};
+  VERIFY( std::find(it, it, 0) == it );
+  return 0;
+}
diff --git a/libstdc++-v3/testsuite/23_containers/vector/debug/114316.cc b/libstdc++-v3/testsuite/23_containers/vector/debug/114316.cc
new file mode 100644
index 00000000000..f211cf67b4c
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/vector/debug/114316.cc
@@ -0,0 +1,16 @@
+// { dg-do run { target c++11 } }
+// { dg-require-debug-mode "" }
+
+// PR libstdc++/114316
+
+#include <vector>
+#include <algorithm>
+
+#include <testsuite_hooks.h>
+
+int main()
+{
+  std::vector<int>::iterator it{};
+  VERIFY( std::find(it, it, 0) == it );
+  return 0;
+}

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 5505 bytes --]

On Sat, 16 Mar 2024, 12:16 François Dumont, <frs.dumont@gmail.com> wrote:

> With the patch, sorry.
>
> On 14/03/2024 22:49, François Dumont wrote:
> > Hi
> >
> > This is what I started to do.
> >
> > For now I haven't touch to __cpp_lib_null_iterators definition as
> > _Safe_local_iterator still need some work.
> >
> > libstdc++: Implement N3644 on _Safe_iterator<> [PR114316]
> >
> > Consider range of value-initialized iterators as valid and empty.
> >
> > libstdc++-v3/ChangeLog:
> >
> >         PR libstdc++/114316
> >         * include/debug/safe_iterator.tcc
> > (_Safe_iterator<>::_M_valid_range):
> >         First check if both iterators are value-initialized before
> > checking if
> >         singular.
> >         * testsuite/23_containers/set/debug/114316.cc: New test case.
> >         * testsuite/23_containers/vector/debug/114316.cc: New test case.
> >
> > Tested under Linux x86_64, ok to commit ?
>

OK for trunk, thanks!

I think this is OK to backport to 13 too.

Maybe after this we can define the __cpp_lib_null_itetators macro for debug
mode?


>
> > François
> >
> >
> > On 12/03/2024 10:52, Jonathan Wakely wrote:
> >> On Tue, 12 Mar 2024 at 01:03, Jonathan Wakely <jwakely@redhat.com>
> >> wrote:
> >>> On Tue, 12 Mar 2024 at 00:55, Maciej Miera <maciej.miera@gmail.com>
> >>> wrote:
> >>>>
> >>>>
> >>>> Wiadomość napisana przez Jonathan Wakely <jwakely@redhat.com> w
> >>>> dniu 11.03.2024, o godz. 21:40:
> >>>>
> >>>> On Mon, 11 Mar 2024 at 20:07, Maciej Miera <maciej.miera@gmail.com>
> >>>> wrote:
> >>>>
> >>>>
> >>>> Hello,
> >>>>
> >>>> I have tried to introduce an extra level of safety to my codebase
> >>>> and utilize _GLIBCXX_DEBUG in my test builds in order to catch
> >>>> faulty iterators.
> >>>> However, I have encountered the following problem: I would like to
> >>>> utilize singular, value-initialized iterators as an arbitrary "null
> >>>> range”.
> >>>> However, this leads to failed assertions in std:: algorithms taking
> >>>> such range.
> >>>>
> >>>> Consider the following code sample with find_if:
> >>>>
> >>>> #include <map>
> >>>> #include <algorithm>
> >>>> #include <iterator>
> >>>>
> >>>> #ifndef __cpp_lib_null_iterators
> >>>> #warning "Not standard compliant"
> >>>> #endif
> >>>>
> >>>> int main()
> >>>> {
> >>>>     std::multimap<char, int>::iterator it1{};
> >>>>     std::multimap<char, int>::iterator it2{};
> >>>>
> >>>>     (void) (it1==it2); // OK
> >>>>     (void) std::find_if(
> >>>>         it1, it2, [](const auto& el) { return el.second == 8;});
> >>>> }
> >>>>
> >>>> Compiled with -std=c++20 and -D_GLIBCXX_DEBUG it produces the
> >>>> warning "Not standard compliant"
> >>>> and the execution results in the following assert failure:
> >>>>
> >>>>
> /opt/compiler-explorer/gcc-12.2.0/include/c++/12.2.0/bits/stl_algo.h:3875:
> >>>>
> >>>> In function:
> >>>>     constexpr _IIter std::find_if(_IIter, _IIter, _Predicate) [with
> >>>> _IIter =
> >>>> gnu_debug::_Safe_iterator<_Rb_tree_iterator<pair<const char, int> >,
> >>>>     debug::multimap<char, int>, bidirectional_iterator_tag>;
> >>>> _Predicate =
> >>>>     main()::<lambda(const auto:16&)>]
> >>>>
> >>>> The question is though: is it by design, or is it just a mere
> >>>> oversight? The warning actually suggest the first option.
> >>>> If it is an intentional design choice, could you provide some
> >>>> rationale behind it, please?
> >>>>
> >>>>
> >>>> The macro was not defined because the C++14 rule wasn't implemented
> >>>> for debug mode, but that should have been fixed for GCC 11, according
> >>>> to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98466 and
> >>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70303
> >>>> So we should be able to define macro now, except maybe it wasn't fixed
> >>>> for the RB tree containers.
> >>>>
> >>>>
> >>>>
> >>>> Just to make sure there are no misunderstandings: comparison via ==
> >>>> works fine. The feature check macro without _GLIBCXX_DEBUG and with
> >>>> <iterator> included is also fine. Maybe the need to include a
> >>>> header is the issue, but that’s not the core of the problem anyway.
> >>> No, it has nothing to do with the headers included. The feature test
> >>> macro is defined like so:
> >>>
> >>> # if (__cplusplus >= 201402L) && (!defined(_GLIBCXX_DEBUG))
> >>> #  define __glibcxx_null_iterators 201304L
> >>>
> >>> It's a very deliberate choice to not define it when _GLIBCXX_DEBUG is
> >>> defined. But as I said, I think we should have changed that.
> >>>
> >>>> The actual question is though, whether passing singular iterators
> >>>> to std algorithms (like find_if) should make the asserts at the
> >>>> beginning of the algo function fail when compiled with
> >>>> _GLIBCXX_DEBUG. IMHO, intuitively it should not, as comparing
> >>>> iterators equal would just ensure early exit and return of the same
> >>>> singular iterator.
> >>>> This seems not to be the case though. The actual message is this:
> >>>> Error: the function requires a valid iterator range [first, last).
> >>>> What bothers me is whether the empty virtual range limited by two
> >>>> same singular iterators is actually valid or not.
> >>> Yes, it's valid. So the bug is in the __glibcxx_requires_valid_range
> >>> macro.
> >> Thanks for the bugzilla report:
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114316
> >> We'll get it fixed!
> >>

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1082 bytes --]


>
> OK for trunk, thanks!
>
> I think this is OK to backport to 13 too.
>
> Maybe after this we can define the __cpp_lib_null_itetators macro for 
> debug mode?
>
After this fix of local_iterator I think we can indeed.

In fact the added 11316.cc was already passing for 
unordered_set<>::local_iterator but simply because we were missing the 
singular check. Both issues solved with this patch.

I found the version.def file to cleanup but no idea how to regenerate 
version.h from it so I'll let you do it, ok ?

     libstdc++: Fix _Safe_local_iterator<>::_M_valid_range

     Unordered container local_iterator range shall not contain any singular
     iterator unless both iterators are value-initialized.

     libstdc++-v3/ChangeLog:

             * include/debug/safe_local_iterator.tcc
             (_Safe_local_iterator::_M_valid_range): Add 
_M_value_initialized and
             _M_singular checks.
             * testsuite/23_containers/unordered_set/debug/114316.cc: 
New test case.


Ok to commit ?

François

[-- Attachment #2: safe_local_iterator.txt --]
[-- Type: text/plain, Size: 1491 bytes --]

diff --git a/libstdc++-v3/include/debug/safe_local_iterator.tcc b/libstdc++-v3/include/debug/safe_local_iterator.tcc
index 90e60e37c32..6d546ec040c 100644
--- a/libstdc++-v3/include/debug/safe_local_iterator.tcc
+++ b/libstdc++-v3/include/debug/safe_local_iterator.tcc
@@ -78,7 +78,13 @@ namespace __gnu_debug
     _M_valid_range(const _Safe_local_iterator& __rhs,
 		std::pair<difference_type, _Distance_precision>& __dist) const
     {
-      if (!_M_can_compare(__rhs))
+      if (_M_value_initialized() && __rhs._M_value_initialized())
+	{
+	  __dist = { 0, __dp_exact };
+	  return true;
+	}
+
+      if (_M_singular() || __rhs._M_singular() || !_M_can_compare(__rhs))
 	return false;
 
       if (bucket() != __rhs.bucket())
diff --git a/libstdc++-v3/testsuite/23_containers/unordered_set/debug/114316.cc b/libstdc++-v3/testsuite/23_containers/unordered_set/debug/114316.cc
new file mode 100644
index 00000000000..41b649a9cbd
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/unordered_set/debug/114316.cc
@@ -0,0 +1,28 @@
+// { dg-do run { target c++11 } }
+// { dg-require-debug-mode "" }
+
+// PR libstdc++/114316
+
+#include <unordered_set>
+#include <algorithm>
+
+#include <testsuite_hooks.h>
+
+void test01()
+{
+  std::unordered_set<int>::iterator it{};
+  VERIFY( std::find(it, it, 0) == it );
+}
+
+void test02()
+{
+  std::unordered_set<int>::local_iterator it{};
+  VERIFY( std::find(it, it, 0) == it );
+}
+
+int main()
+{
+  test01();
+  test02();
+  return 0;
+}

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1865 bytes --]

I was a little bit too confident below. After review of all _M_singular 
usages I found another necessary fix.

After this one for sure we will be able to define 
__cpp_lib_null_iterators even in Debug mode.

     libstdc++: Fix N3344 behavior on _Safe_iterator::_M_can_advance

     We shall be able to advance from a 0 offset a value-initialized 
iterator.

     libstdc++-v3/ChangeLog:

             * include/debug/safe_iterator.tcc 
(_Safe_iterator<>::_M_can_advance):
             Accept 0 offset advance on value-initialized iterator.
             * testsuite/23_containers/vector/debug/n3644.cc: New test case.

Ok to commit ?

François


On 17/03/2024 17:52, François Dumont wrote:
>
>>
>> OK for trunk, thanks!
>>
>> I think this is OK to backport to 13 too.
>>
>> Maybe after this we can define the __cpp_lib_null_itetators macro for 
>> debug mode?
>>
> After this fix of local_iterator I think we can indeed.
>
> In fact the added 11316.cc was already passing for 
> unordered_set<>::local_iterator but simply because we were missing the 
> singular check. Both issues solved with this patch.
>
> I found the version.def file to cleanup but no idea how to regenerate 
> version.h from it so I'll let you do it, ok ?
>
>     libstdc++: Fix _Safe_local_iterator<>::_M_valid_range
>
>     Unordered container local_iterator range shall not contain any 
> singular
>     iterator unless both iterators are value-initialized.
>
>     libstdc++-v3/ChangeLog:
>
>             * include/debug/safe_local_iterator.tcc
>             (_Safe_local_iterator::_M_valid_range): Add 
> _M_value_initialized and
>             _M_singular checks.
>             * testsuite/23_containers/unordered_set/debug/114316.cc: 
> New test case.
>
>
> Ok to commit ?
>
> François

[-- Attachment #2: n3344_patch.txt --]
[-- Type: text/plain, Size: 1088 bytes --]

diff --git a/libstdc++-v3/include/debug/safe_iterator.tcc b/libstdc++-v3/include/debug/safe_iterator.tcc
index 4b2baf2980e..deaa84d0a1f 100644
--- a/libstdc++-v3/include/debug/safe_iterator.tcc
+++ b/libstdc++-v3/include/debug/safe_iterator.tcc
@@ -86,6 +86,9 @@ namespace __gnu_debug
     _Safe_iterator<_Iterator, _Sequence, _Category>::
     _M_can_advance(difference_type __n, bool __strict) const
     {
+      if (this->_M_value_initialized() && __n == 0)
+	return true;
+
       if (this->_M_singular())
 	return false;
 
diff --git a/libstdc++-v3/testsuite/23_containers/vector/debug/n3644.cc b/libstdc++-v3/testsuite/23_containers/vector/debug/n3644.cc
new file mode 100644
index 00000000000..052c52f26b7
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/vector/debug/n3644.cc
@@ -0,0 +1,16 @@
+// { dg-do run { target c++11 } }
+// { dg-require-debug-mode "" }
+
+#include <vector>
+#include <algorithm>
+
+#include <testsuite_hooks.h>
+
+int main()
+{
+  std::vector<int>::iterator it{};
+  auto cpy = it;
+  std::advance(it, 0);
+  VERIFY( it == cpy );
+  return 0;
+}

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[Jonathan Wakely]

On Sun, 17 Mar 2024 at 16:52, François Dumont <frs.dumont@gmail.com> wrote:
>
>
> >
> > OK for trunk, thanks!
> >
> > I think this is OK to backport to 13 too.
> >
> > Maybe after this we can define the __cpp_lib_null_itetators macro for
> > debug mode?
> >
> After this fix of local_iterator I think we can indeed.
>
> In fact the added 11316.cc was already passing for
> unordered_set<>::local_iterator but simply because we were missing the
> singular check. Both issues solved with this patch.
>
> I found the version.def file to cleanup but no idea how to regenerate
> version.h from it so I'll let you do it, ok ?

Sure, I can do that. To regenerate it run 'make update-version' in the
libstdc++-v3/include build directory.

>
>      libstdc++: Fix _Safe_local_iterator<>::_M_valid_range
>
>      Unordered container local_iterator range shall not contain any singular
>      iterator unless both iterators are value-initialized.
>
>      libstdc++-v3/ChangeLog:
>
>              * include/debug/safe_local_iterator.tcc
>              (_Safe_local_iterator::_M_valid_range): Add
> _M_value_initialized and
>              _M_singular checks.
>              * testsuite/23_containers/unordered_set/debug/114316.cc:
> New test case.
>
>
> Ok to commit ?

OK.

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[Jonathan Wakely]

On Sun, 17 Mar 2024 at 18:14, François Dumont <frs.dumont@gmail.com> wrote:
>
> I was a little bit too confident below. After review of all _M_singular
> usages I found another necessary fix.
>
> After this one for sure we will be able to define
> __cpp_lib_null_iterators even in Debug mode.
>
>      libstdc++: Fix N3344 behavior on _Safe_iterator::_M_can_advance
>
>      We shall be able to advance from a 0 offset a value-initialized
> iterator.
>
>      libstdc++-v3/ChangeLog:
>
>              * include/debug/safe_iterator.tcc
> (_Safe_iterator<>::_M_can_advance):
>              Accept 0 offset advance on value-initialized iterator.
>              * testsuite/23_containers/vector/debug/n3644.cc: New test case.
>
> Ok to commit ?


OK, thanks.


> François
>
>
> On 17/03/2024 17:52, François Dumont wrote:
> >
> >>
> >> OK for trunk, thanks!
> >>
> >> I think this is OK to backport to 13 too.
> >>
> >> Maybe after this we can define the __cpp_lib_null_itetators macro for
> >> debug mode?
> >>
> > After this fix of local_iterator I think we can indeed.
> >
> > In fact the added 11316.cc was already passing for
> > unordered_set<>::local_iterator but simply because we were missing the
> > singular check. Both issues solved with this patch.
> >
> > I found the version.def file to cleanup but no idea how to regenerate
> > version.h from it so I'll let you do it, ok ?
> >
> >     libstdc++: Fix _Safe_local_iterator<>::_M_valid_range
> >
> >     Unordered container local_iterator range shall not contain any
> > singular
> >     iterator unless both iterators are value-initialized.
> >
> >     libstdc++-v3/ChangeLog:
> >
> >             * include/debug/safe_local_iterator.tcc
> >             (_Safe_local_iterator::_M_valid_range): Add
> > _M_value_initialized and
> >             _M_singular checks.
> >             * testsuite/23_containers/unordered_set/debug/114316.cc:
> > New test case.
> >
> >
> > Ok to commit ?
> >
> > François

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[François Dumont]

Both committed now.

Just to confirm, those 2 last patches should be backported to gcc-13 
branch, right ?

I might have a try to update version.h but if you want to do it before 
don't hesitate.

François

On 18/03/2024 08:45, Jonathan Wakely wrote:
> On Sun, 17 Mar 2024 at 18:14, François Dumont <frs.dumont@gmail.com> wrote:
>> I was a little bit too confident below. After review of all _M_singular
>> usages I found another necessary fix.
>>
>> After this one for sure we will be able to define
>> __cpp_lib_null_iterators even in Debug mode.
>>
>>       libstdc++: Fix N3344 behavior on _Safe_iterator::_M_can_advance
>>
>>       We shall be able to advance from a 0 offset a value-initialized
>> iterator.
>>
>>       libstdc++-v3/ChangeLog:
>>
>>               * include/debug/safe_iterator.tcc
>> (_Safe_iterator<>::_M_can_advance):
>>               Accept 0 offset advance on value-initialized iterator.
>>               * testsuite/23_containers/vector/debug/n3644.cc: New test case.
>>
>> Ok to commit ?
>
> OK, thanks.
>
>
>> François
>>
>>
>> On 17/03/2024 17:52, François Dumont wrote:
>>>> OK for trunk, thanks!
>>>>
>>>> I think this is OK to backport to 13 too.
>>>>
>>>> Maybe after this we can define the __cpp_lib_null_itetators macro for
>>>> debug mode?
>>>>
>>> After this fix of local_iterator I think we can indeed.
>>>
>>> In fact the added 11316.cc was already passing for
>>> unordered_set<>::local_iterator but simply because we were missing the
>>> singular check. Both issues solved with this patch.
>>>
>>> I found the version.def file to cleanup but no idea how to regenerate
>>> version.h from it so I'll let you do it, ok ?
>>>
>>>      libstdc++: Fix _Safe_local_iterator<>::_M_valid_range
>>>
>>>      Unordered container local_iterator range shall not contain any
>>> singular
>>>      iterator unless both iterators are value-initialized.
>>>
>>>      libstdc++-v3/ChangeLog:
>>>
>>>              * include/debug/safe_local_iterator.tcc
>>>              (_Safe_local_iterator::_M_valid_range): Add
>>> _M_value_initialized and
>>>              _M_singular checks.
>>>              * testsuite/23_containers/unordered_set/debug/114316.cc:
>>> New test case.
>>>
>>>
>>> Ok to commit ?
>>>
>>> François

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[Jonathan Wakely]

On Mon, 18 Mar 2024 at 21:38, François Dumont wrote:
>
> Both committed now.
>
> Just to confirm, those 2 last patches should be backported to gcc-13
> branch, right ?

Yes please.

>
> I might have a try to update version.h but if you want to do it before
> don't hesitate.

You'll need to have 'autogen' installed to do it (I'm going to update
the docs to describe that today).

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[Jonathan Wakely]

On Tue, 19 Mar 2024 at 09:31, Jonathan Wakely wrote:
>
> On Mon, 18 Mar 2024 at 21:38, François Dumont wrote:
> >
> > Both committed now.
> >
> > Just to confirm, those 2 last patches should be backported to gcc-13
> > branch, right ?
>
> Yes please.
>
> >
> > I might have a try to update version.h but if you want to do it before
> > don't hesitate.
>
> You'll need to have 'autogen' installed to do it (I'm going to update
> the docs to describe that today).

I've documented it in a new "Generated files" section at the end of
https://gcc.gnu.org/onlinedocs/libstdc++/manual/appendix_porting.html#build_hacking.generated

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

Thanks to you doc:

     libstdc++: [_GLIBCXX_DEBUG] Define __[glibcxx,cpp_lib]_null_iterators

     _GLIBCXX_DEBUG has now fully N3344 compliant iterator checks, we 
can define
     __glibcxx_null_iterators and __cpp_lib_null_iterators macros like 
the normal
     mode.

     libstdc++-v3/ChangeLog:

             * version.def (null_iterators): Remove extra_cond.
             * version.h: Regenerate.

Ok to commit ?

I already noticed that GCC 13 has no version.h file so no backport question.

François

On 19/03/2024 16:41, Jonathan Wakely wrote:
> On Tue, 19 Mar 2024 at 09:31, Jonathan Wakely wrote:
>> On Mon, 18 Mar 2024 at 21:38, François Dumont wrote:
>>> Both committed now.
>>>
>>> Just to confirm, those 2 last patches should be backported to gcc-13
>>> branch, right ?
>> Yes please.
>>
>>> I might have a try to update version.h but if you want to do it before
>>> don't hesitate.
>> You'll need to have 'autogen' installed to do it (I'm going to update
>> the docs to describe that today).
> I've documented it in a new "Generated files" section at the end of
> https://gcc.gnu.org/onlinedocs/libstdc++/manual/appendix_porting.html#build_hacking.generated
>

[-- Attachment #2: version_patch.txt --]
[-- Type: text/plain, Size: 927 bytes --]

diff --git a/libstdc++-v3/include/bits/version.def b/libstdc++-v3/include/bits/version.def
index be5af18e818..26e62c6a9b2 100644
--- a/libstdc++-v3/include/bits/version.def
+++ b/libstdc++-v3/include/bits/version.def
@@ -209,7 +209,6 @@ ftms = {
   values = {
     v = 201304;
     cxxmin = 14;
-    extra_cond = "!defined(_GLIBCXX_DEBUG)";
   };
 };
 
diff --git a/libstdc++-v3/include/bits/version.h b/libstdc++-v3/include/bits/version.h
index 9107b45a484..23c8c09ab4b 100644
--- a/libstdc++-v3/include/bits/version.h
+++ b/libstdc++-v3/include/bits/version.h
@@ -214,7 +214,7 @@
 #undef __glibcxx_want_make_reverse_iterator
 
 #if !defined(__cpp_lib_null_iterators)
-# if (__cplusplus >= 201402L) && (!defined(_GLIBCXX_DEBUG))
+# if (__cplusplus >= 201402L)
 #  define __glibcxx_null_iterators 201304L
 #  if defined(__glibcxx_want_all) || defined(__glibcxx_want_null_iterators)
 #   define __cpp_lib_null_iterators 201304L

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[Jonathan Wakely]

On Wed, 20 Mar 2024 at 05:59, François Dumont wrote:
>
> Thanks to you doc:
>
>      libstdc++: [_GLIBCXX_DEBUG] Define __[glibcxx,cpp_lib]_null_iterators
>
>      _GLIBCXX_DEBUG has now fully N3344 compliant iterator checks, we
> can define
>      __glibcxx_null_iterators and __cpp_lib_null_iterators macros like
> the normal
>      mode.
>
>      libstdc++-v3/ChangeLog:
>
>              * version.def (null_iterators): Remove extra_cond.
>              * version.h: Regenerate.
>
> Ok to commit ?

Please don't bother talking about __glibcxx_null_iterators in the
commit message, that's an implementation detail that always mirrors
the standard-defined __cpp_lib_null_iterators one. The first line of
the commit will be much easier to read without that.

OK with that change, thanks.

> I already noticed that GCC 13 has no version.h file so no backport question.

It has no version.h but it still has the macros:

include/std/iterator:# define __cpp_lib_null_iterators 201304L
include/std/version:# define __cpp_lib_null_iterators 201304L

Those definitions can be made to not depend on _GLIBCXX_DEBUG.

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]

As proposed below I also updated gcc-13 branch.

     libstdc++: [_GLIBCXX_DEBUG] Define __cpp_lib_null_iterators

     _GLIBCXX_DEBUG has now fully N3344 compliant iterator checks, we 
can define
     __cpp_lib_null_iterators macros like the normal mode.

     libstdc++-v3/ChangeLog:

             * include/std/iterator (__cpp_lib_null_iterators): Define 
regardless of
             _GLIBCXX_DEBUG.
             * include/std/version (__cpp_lib_null_iterators): Likewise.

François


On 20/03/2024 10:02, Jonathan Wakely wrote:
> On Wed, 20 Mar 2024 at 05:59, François Dumont wrote:
>> Thanks to you doc:
>>
>>       libstdc++: [_GLIBCXX_DEBUG] Define __[glibcxx,cpp_lib]_null_iterators
>>
>>       _GLIBCXX_DEBUG has now fully N3344 compliant iterator checks, we
>> can define
>>       __glibcxx_null_iterators and __cpp_lib_null_iterators macros like
>> the normal
>>       mode.
>>
>>       libstdc++-v3/ChangeLog:
>>
>>               * version.def (null_iterators): Remove extra_cond.
>>               * version.h: Regenerate.
>>
>> Ok to commit ?
> Please don't bother talking about __glibcxx_null_iterators in the
> commit message, that's an implementation detail that always mirrors
> the standard-defined __cpp_lib_null_iterators one. The first line of
> the commit will be much easier to read without that.
>
> OK with that change, thanks.
>
>> I already noticed that GCC 13 has no version.h file so no backport question.
> It has no version.h but it still has the macros:
>
> include/std/iterator:# define __cpp_lib_null_iterators 201304L
> include/std/version:# define __cpp_lib_null_iterators 201304L
>
> Those definitions can be made to not depend on _GLIBCXX_DEBUG.
>

[-- Attachment #2: version_patch.txt --]
[-- Type: text/plain, Size: 1073 bytes --]

diff --git a/libstdc++-v3/include/std/iterator b/libstdc++-v3/include/std/iterator
index 695e18e2c47..a0a8eac570b 100644
--- a/libstdc++-v3/include/std/iterator
+++ b/libstdc++-v3/include/std/iterator
@@ -67,7 +67,7 @@
 #endif
 #include <bits/range_access.h>
 
-#if __cplusplus >= 201402L && ! defined _GLIBCXX_DEBUG // PR libstdc++/70303
+#if __cplusplus >= 201402L
 # define __cpp_lib_null_iterators 201304L
 #endif
 
diff --git a/libstdc++-v3/include/std/version b/libstdc++-v3/include/std/version
index bd1bee0190d..ee515c4e66c 100644
--- a/libstdc++-v3/include/std/version
+++ b/libstdc++-v3/include/std/version
@@ -81,9 +81,7 @@
 #define __cpp_lib_integral_constant_callable 201304L
 #define __cpp_lib_is_final 201402L
 #define __cpp_lib_make_reverse_iterator 201402L
-#ifndef _GLIBCXX_DEBUG // PR libstdc++/70303
-# define __cpp_lib_null_iterators 201304L
-#endif
+#define __cpp_lib_null_iterators 201304L
 #define __cpp_lib_robust_nonmodifying_seq_ops 201304L
 #define __cpp_lib_transformation_trait_aliases 201304L
 #define __cpp_lib_transparent_operators 201510L

Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316]

[Jonathan Wakely]

On Wed, 20 Mar 2024 at 18:11, François Dumont wrote:
>
> As proposed below I also updated gcc-13 branch.

Great, thanks.

>
>      libstdc++: [_GLIBCXX_DEBUG] Define __cpp_lib_null_iterators
>
>      _GLIBCXX_DEBUG has now fully N3344 compliant iterator checks, we
> can define
>      __cpp_lib_null_iterators macros like the normal mode.
>
>      libstdc++-v3/ChangeLog:
>
>              * include/std/iterator (__cpp_lib_null_iterators): Define
> regardless of
>              _GLIBCXX_DEBUG.
>              * include/std/version (__cpp_lib_null_iterators): Likewise.
>
> François
>
>
> On 20/03/2024 10:02, Jonathan Wakely wrote:
> > On Wed, 20 Mar 2024 at 05:59, François Dumont wrote:
> >> Thanks to you doc:
> >>
> >>       libstdc++: [_GLIBCXX_DEBUG] Define __[glibcxx,cpp_lib]_null_iterators
> >>
> >>       _GLIBCXX_DEBUG has now fully N3344 compliant iterator checks, we
> >> can define
> >>       __glibcxx_null_iterators and __cpp_lib_null_iterators macros like
> >> the normal
> >>       mode.
> >>
> >>       libstdc++-v3/ChangeLog:
> >>
> >>               * version.def (null_iterators): Remove extra_cond.
> >>               * version.h: Regenerate.
> >>
> >> Ok to commit ?
> > Please don't bother talking about __glibcxx_null_iterators in the
> > commit message, that's an implementation detail that always mirrors
> > the standard-defined __cpp_lib_null_iterators one. The first line of
> > the commit will be much easier to read without that.
> >
> > OK with that change, thanks.
> >
> >> I already noticed that GCC 13 has no version.h file so no backport question.
> > It has no version.h but it still has the macros:
> >
> > include/std/iterator:# define __cpp_lib_null_iterators 201304L
> > include/std/version:# define __cpp_lib_null_iterators 201304L
> >
> > Those definitions can be made to not depend on _GLIBCXX_DEBUG.
> >