* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] [not found] ` <CACb0b4kQAvpDkWVW00GwPU+ve3T-TMQxhZrJSKs9jzYK+zN=DQ@mail.gmail.com> @ 2024-03-14 21:49 ` François Dumont 2024-03-16 12:16 ` François Dumont 0 siblings, 1 reply; 14+ messages in thread From: François Dumont @ 2024-03-14 21:49 UTC (permalink / raw) To: Jonathan Wakely, Maciej Miera; +Cc: libstdc++, gcc-patches 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! > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-14 21:49 ` _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] François Dumont @ 2024-03-16 12:16 ` François Dumont 2024-03-17 11:45 ` Jonathan Wakely 0 siblings, 1 reply; 14+ messages in thread From: François Dumont @ 2024-03-16 12:16 UTC (permalink / raw) To: Jonathan Wakely, Maciej Miera; +Cc: libstdc++, gcc-patches [-- 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; +} ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-16 12:16 ` François Dumont @ 2024-03-17 11:45 ` Jonathan Wakely 2024-03-17 16:52 ` François Dumont 0 siblings, 1 reply; 14+ messages in thread From: Jonathan Wakely @ 2024-03-17 11:45 UTC (permalink / raw) To: François Dumont Cc: Jonathan Wakely, Maciej Miera, libstdc++, gcc-patches [-- 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! > >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-17 11:45 ` Jonathan Wakely @ 2024-03-17 16:52 ` François Dumont 2024-03-17 18:14 ` François Dumont 2024-03-18 7:45 ` Jonathan Wakely 0 siblings, 2 replies; 14+ messages in thread From: François Dumont @ 2024-03-17 16:52 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Jonathan Wakely, libstdc++, gcc-patches [-- 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; +} ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-17 16:52 ` François Dumont @ 2024-03-17 18:14 ` François Dumont 2024-03-18 7:45 ` Jonathan Wakely 2024-03-18 7:45 ` Jonathan Wakely 1 sibling, 1 reply; 14+ messages in thread From: François Dumont @ 2024-03-17 18:14 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Jonathan Wakely, libstdc++, gcc-patches [-- 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; +} ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-17 18:14 ` François Dumont @ 2024-03-18 7:45 ` Jonathan Wakely 2024-03-18 21:38 ` François Dumont 0 siblings, 1 reply; 14+ messages in thread From: Jonathan Wakely @ 2024-03-18 7:45 UTC (permalink / raw) To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-18 7:45 ` Jonathan Wakely @ 2024-03-18 21:38 ` François Dumont 2024-03-19 9:31 ` Jonathan Wakely 0 siblings, 1 reply; 14+ messages in thread From: François Dumont @ 2024-03-18 21:38 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Jonathan Wakely, libstdc++, gcc-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-18 21:38 ` François Dumont @ 2024-03-19 9:31 ` Jonathan Wakely 2024-03-19 15:41 ` Jonathan Wakely 0 siblings, 1 reply; 14+ messages in thread From: Jonathan Wakely @ 2024-03-19 9:31 UTC (permalink / raw) To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches 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). ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-19 9:31 ` Jonathan Wakely @ 2024-03-19 15:41 ` Jonathan Wakely 2024-03-20 5:59 ` François Dumont 0 siblings, 1 reply; 14+ messages in thread From: Jonathan Wakely @ 2024-03-19 15:41 UTC (permalink / raw) To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-19 15:41 ` Jonathan Wakely @ 2024-03-20 5:59 ` François Dumont 2024-03-20 9:02 ` Jonathan Wakely 0 siblings, 1 reply; 14+ messages in thread From: François Dumont @ 2024-03-20 5:59 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Jonathan Wakely, libstdc++, gcc-patches [-- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-20 5:59 ` François Dumont @ 2024-03-20 9:02 ` Jonathan Wakely 2024-03-20 18:10 ` François Dumont 0 siblings, 1 reply; 14+ messages in thread From: Jonathan Wakely @ 2024-03-20 9:02 UTC (permalink / raw) To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-20 9:02 ` Jonathan Wakely @ 2024-03-20 18:10 ` François Dumont 2024-03-21 6:20 ` Jonathan Wakely 0 siblings, 1 reply; 14+ messages in thread From: François Dumont @ 2024-03-20 18:10 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Jonathan Wakely, libstdc++, gcc-patches [-- 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-20 18:10 ` François Dumont @ 2024-03-21 6:20 ` Jonathan Wakely 0 siblings, 0 replies; 14+ messages in thread From: Jonathan Wakely @ 2024-03-21 6:20 UTC (permalink / raw) To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches 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. > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] 2024-03-17 16:52 ` François Dumont 2024-03-17 18:14 ` François Dumont @ 2024-03-18 7:45 ` Jonathan Wakely 1 sibling, 0 replies; 14+ messages in thread From: Jonathan Wakely @ 2024-03-18 7:45 UTC (permalink / raw) To: François Dumont; +Cc: Jonathan Wakely, libstdc++, gcc-patches 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-03-21 6:20 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <73AC0523-2237-46FD-9885-7AE3F8663DF2@gmail.com>
[not found] ` <CACb0b4=K2UNWaqRa884LWxk29KLqi7MwOXcNUq+a4rm6-a8rsw@mail.gmail.com>
[not found] ` <28CE4FD1-FFB0-4300-81CA-C3CB07E436A6@gmail.com>
[not found] ` <CACb0b4nbBPtsKybYBS6V=HwQur0Kbjg+nYpCDXAd8x0Bg6pWvw@mail.gmail.com>
[not found] ` <CACb0b4kQAvpDkWVW00GwPU+ve3T-TMQxhZrJSKs9jzYK+zN=DQ@mail.gmail.com>
2024-03-14 21:49 ` _LIBCXX_DEBUG value initialized singular iterators assert failures in std algorithms [PR104316] François Dumont
2024-03-16 12:16 ` François Dumont
2024-03-17 11:45 ` Jonathan Wakely
2024-03-17 16:52 ` François Dumont
2024-03-17 18:14 ` François Dumont
2024-03-18 7:45 ` Jonathan Wakely
2024-03-18 21:38 ` François Dumont
2024-03-19 9:31 ` Jonathan Wakely
2024-03-19 15:41 ` Jonathan Wakely
2024-03-20 5:59 ` François Dumont
2024-03-20 9:02 ` Jonathan Wakely
2024-03-20 18:10 ` François Dumont
2024-03-21 6:20 ` Jonathan Wakely
2024-03-18 7:45 ` Jonathan Wakely
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).