Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check
On 27/11/19 06:37 +0100, François Dumont wrote: On 11/26/19 10:52 PM, Jonathan Wakely wrote: On 26/11/19 20:07 +0100, François Dumont wrote: Sure, I am about to do so. However I wasn't sure about this syntax before the commit so I had run the new 2_neg.cc with: make CXXFLAGS=-std=c++98 check-debug and it worked fine and still is ! The testsuite doesn't use CXXFLAGS. Did you mean CPPFLAGS ? No, I mean CXXFLAGS. I do see the option added to the compiler call in libstdc++.log file. And I used it to build for instance pretty-printers tests in _GLIBCXX_DEBUG mode with success. Ah, it works if you run it *inside* the $target/libstdc++-v3/testsuite directory. I usually run the tests from the parent directory. I also try -std=gnu++98 and made sure that pch had been updated by re-building libstdc++ first. I just can't get the expected compilation error. Maybe I need to rebuild the whole stuff to get an error... No, you need to pass the right flags so they are used by the testsuite. This will do it: make -C testsuite/ check-debug debug_flags=unix/-D_GLIBCXX_DEBUG/-std=gnu++98/-Wsystem-headers I'll have to keep your mail to remember it ! Just look in testsuite/Makefile to see what variables are used by the check-debug target. That's what determines the flags used, not my email! :-) But since it only shows up with -Wsystem-headers, there's no point trying to test for it. Ok, makes sens. Thanks
Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check
On 11/26/19 10:52 PM, Jonathan Wakely wrote: On 26/11/19 20:07 +0100, François Dumont wrote: Sure, I am about to do so. However I wasn't sure about this syntax before the commit so I had run the new 2_neg.cc with: make CXXFLAGS=-std=c++98 check-debug and it worked fine and still is ! The testsuite doesn't use CXXFLAGS. Did you mean CPPFLAGS ? I do see the option added to the compiler call in libstdc++.log file. And I used it to build for instance pretty-printers tests in _GLIBCXX_DEBUG mode with success. I also try -std=gnu++98 and made sure that pch had been updated by re-building libstdc++ first. I just can't get the expected compilation error. Maybe I need to rebuild the whole stuff to get an error... No, you need to pass the right flags so they are used by the testsuite. This will do it: make -C testsuite/ check-debug debug_flags=unix/-D_GLIBCXX_DEBUG/-std=gnu++98/-Wsystem-headers I'll have to keep your mail to remember it ! But since it only shows up with -Wsystem-headers, there's no point trying to test for it. Ok, makes sens. Thanks
Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check
On 26/11/19 20:07 +0100, François Dumont wrote: On 11/26/19 1:21 PM, Jonathan Wakely wrote: On 26/11/19 12:33 +0100, Stephan Bergmann wrote: On 22/11/2019 18:59, Jonathan Wakely wrote: On 22/11/19 18:38 +0100, François Dumont wrote: I noticed that we are not checking that iterators are not singular in valid_range. Moreover __check_singular signature for pointers is not intercepting all kind of pointers in terms of qualification. I'd like to commit it next week but considering we are in stage 3 I need proper acceptance. * include/debug/functions.h: Remove include. (__check_singular_aux, __check_singular): Move... * include/debug/helper_functions.h: (__check_singular_aux, __check_singular): ...here. (__valid_range_aux): Adapt to use latter. * testsuite/25_algorithms/copy/debug/2_neg.cc: New. Tested under Linux x86_64 normal and debug modes. OK for trunk, thanks. The curly braces... diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h index c3e7478f649..5a858754875 100644 --- a/libstdc++-v3/include/debug/helper_functions.h +++ b/libstdc++-v3/include/debug/helper_functions.h [...] @@ -138,14 +156,23 @@ namespace __gnu_debug inline bool __valid_range_aux(_InputIterator __first, _InputIterator __last, std::input_iterator_tag) - { return true; } + { + if (__first != __last) + return !__check_singular(__first) && !__check_singular(__last); + + return true; + } template _GLIBCXX_CONSTEXPR inline bool __valid_range_aux(_InputIterator __first, _InputIterator __last, std::random_access_iterator_tag) - { return __first <= __last; } + { + return + __valid_range_aux(__first, __last, std::input_iterator_tag{}) ...^^^ here... + && __first <= __last; + } /** We have iterators, so figure out what kind of iterators they are * to see if we can check the range ahead of time. @@ -167,6 +194,9 @@ namespace __gnu_debug typename _Distance_traits<_InputIterator>::__type& __dist, std::__false_type) { + if (!__valid_range_aux(__first, __last, std::input_iterator_tag{})) ...and ^^^ here are not allowed pre C++11. Replacing those with std::input_iterator_tag() should fix it. Indeed. We should also have tests that use "-std=gnu++98 -D_GLIBCXX_DEBUG" so they'd catch this. François, can you take care of the fix please? Sure, I am about to do so. However I wasn't sure about this syntax before the commit so I had run the new 2_neg.cc with: make CXXFLAGS=-std=c++98 check-debug and it worked fine and still is ! The testsuite doesn't use CXXFLAGS. I also try -std=gnu++98 and made sure that pch had been updated by re-building libstdc++ first. I just can't get the expected compilation error. Maybe I need to rebuild the whole stuff to get an error... No, you need to pass the right flags so they are used by the testsuite. This will do it: make -C testsuite/ check-debug debug_flags=unix/-D_GLIBCXX_DEBUG/-std=gnu++98/-Wsystem-headers But since it only shows up with -Wsystem-headers, there's no point trying to test for it.
Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check
On 11/26/19 9:49 PM, Stephan Bergmann wrote: On 26/11/2019 20:07, François Dumont wrote: However I wasn't sure about this syntax before the commit so I had run the new 2_neg.cc with: make CXXFLAGS=-std=c++98 check-debug and it worked fine and still is ! I also try -std=gnu++98 and made sure that pch had been updated by re-building libstdc++ first. I just can't get the expected compilation error. Maybe I need to rebuild the whole stuff to get an error... I saw the error with recent Clang and -std=gnu++98. Recent GCC indeed seems to give at most a warning. Ok, thanks for the feedback, gcc might be more permissive then. Whatever I committed the fix an hour ago.
Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check
On 26/11/2019 20:07, François Dumont wrote: However I wasn't sure about this syntax before the commit so I had run the new 2_neg.cc with: make CXXFLAGS=-std=c++98 check-debug and it worked fine and still is ! I also try -std=gnu++98 and made sure that pch had been updated by re-building libstdc++ first. I just can't get the expected compilation error. Maybe I need to rebuild the whole stuff to get an error... I saw the error with recent Clang and -std=gnu++98. Recent GCC indeed seems to give at most a warning.
Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check
On 11/26/19 1:21 PM, Jonathan Wakely wrote: On 26/11/19 12:33 +0100, Stephan Bergmann wrote: On 22/11/2019 18:59, Jonathan Wakely wrote: On 22/11/19 18:38 +0100, François Dumont wrote: I noticed that we are not checking that iterators are not singular in valid_range. Moreover __check_singular signature for pointers is not intercepting all kind of pointers in terms of qualification. I'd like to commit it next week but considering we are in stage 3 I need proper acceptance. * include/debug/functions.h: Remove include. (__check_singular_aux, __check_singular): Move... * include/debug/helper_functions.h: (__check_singular_aux, __check_singular): ...here. (__valid_range_aux): Adapt to use latter. * testsuite/25_algorithms/copy/debug/2_neg.cc: New. Tested under Linux x86_64 normal and debug modes. OK for trunk, thanks. The curly braces... diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h index c3e7478f649..5a858754875 100644 --- a/libstdc++-v3/include/debug/helper_functions.h +++ b/libstdc++-v3/include/debug/helper_functions.h [...] @@ -138,14 +156,23 @@ namespace __gnu_debug inline bool __valid_range_aux(_InputIterator __first, _InputIterator __last, std::input_iterator_tag) - { return true; } + { + if (__first != __last) + return !__check_singular(__first) && !__check_singular(__last); + + return true; + } template _GLIBCXX_CONSTEXPR inline bool __valid_range_aux(_InputIterator __first, _InputIterator __last, std::random_access_iterator_tag) - { return __first <= __last; } + { + return + __valid_range_aux(__first, __last, std::input_iterator_tag{}) ...^^^ here... + && __first <= __last; + } /** We have iterators, so figure out what kind of iterators they are * to see if we can check the range ahead of time. @@ -167,6 +194,9 @@ namespace __gnu_debug typename _Distance_traits<_InputIterator>::__type& __dist, std::__false_type) { + if (!__valid_range_aux(__first, __last, std::input_iterator_tag{})) ...and ^^^ here are not allowed pre C++11. Replacing those with std::input_iterator_tag() should fix it. Indeed. We should also have tests that use "-std=gnu++98 -D_GLIBCXX_DEBUG" so they'd catch this. François, can you take care of the fix please? Sure, I am about to do so. However I wasn't sure about this syntax before the commit so I had run the new 2_neg.cc with: make CXXFLAGS=-std=c++98 check-debug and it worked fine and still is ! I also try -std=gnu++98 and made sure that pch had been updated by re-building libstdc++ first. I just can't get the expected compilation error. Maybe I need to rebuild the whole stuff to get an error... Sorry
Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check
On 26/11/19 12:33 +0100, Stephan Bergmann wrote: On 22/11/2019 18:59, Jonathan Wakely wrote: On 22/11/19 18:38 +0100, François Dumont wrote: I noticed that we are not checking that iterators are not singular in valid_range. Moreover __check_singular signature for pointers is not intercepting all kind of pointers in terms of qualification. I'd like to commit it next week but considering we are in stage 3 I need proper acceptance. * include/debug/functions.h: Remove include. (__check_singular_aux, __check_singular): Move... * include/debug/helper_functions.h: (__check_singular_aux, __check_singular): ...here. (__valid_range_aux): Adapt to use latter. * testsuite/25_algorithms/copy/debug/2_neg.cc: New. Tested under Linux x86_64 normal and debug modes. OK for trunk, thanks. The curly braces... diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h index c3e7478f649..5a858754875 100644 --- a/libstdc++-v3/include/debug/helper_functions.h +++ b/libstdc++-v3/include/debug/helper_functions.h [...] @@ -138,14 +156,23 @@ namespace __gnu_debug inline bool __valid_range_aux(_InputIterator __first, _InputIterator __last, std::input_iterator_tag) -{ return true; } +{ + if (__first != __last) + return !__check_singular(__first) && !__check_singular(__last); + + return true; +} template _GLIBCXX_CONSTEXPR inline bool __valid_range_aux(_InputIterator __first, _InputIterator __last, std::random_access_iterator_tag) -{ return __first <= __last; } +{ + return + __valid_range_aux(__first, __last, std::input_iterator_tag{}) ...^^^ here... + && __first <= __last; +} /** We have iterators, so figure out what kind of iterators they are * to see if we can check the range ahead of time. @@ -167,6 +194,9 @@ namespace __gnu_debug typename _Distance_traits<_InputIterator>::__type& __dist, std::__false_type) { + if (!__valid_range_aux(__first, __last, std::input_iterator_tag{})) ...and ^^^ here are not allowed pre C++11. Replacing those with std::input_iterator_tag() should fix it. Indeed. We should also have tests that use "-std=gnu++98 -D_GLIBCXX_DEBUG" so they'd catch this. François, can you take care of the fix please?
Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check
On 22/11/2019 18:59, Jonathan Wakely wrote: On 22/11/19 18:38 +0100, François Dumont wrote: I noticed that we are not checking that iterators are not singular in valid_range. Moreover __check_singular signature for pointers is not intercepting all kind of pointers in terms of qualification. I'd like to commit it next week but considering we are in stage 3 I need proper acceptance. * include/debug/functions.h: Remove include. (__check_singular_aux, __check_singular): Move... * include/debug/helper_functions.h: (__check_singular_aux, __check_singular): ...here. (__valid_range_aux): Adapt to use latter. * testsuite/25_algorithms/copy/debug/2_neg.cc: New. Tested under Linux x86_64 normal and debug modes. OK for trunk, thanks. The curly braces... diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h index c3e7478f649..5a858754875 100644 --- a/libstdc++-v3/include/debug/helper_functions.h +++ b/libstdc++-v3/include/debug/helper_functions.h [...] @@ -138,14 +156,23 @@ namespace __gnu_debug inline bool __valid_range_aux(_InputIterator __first, _InputIterator __last, std::input_iterator_tag) -{ return true; } +{ + if (__first != __last) + return !__check_singular(__first) && !__check_singular(__last); + + return true; +} template _GLIBCXX_CONSTEXPR inline bool __valid_range_aux(_InputIterator __first, _InputIterator __last, std::random_access_iterator_tag) -{ return __first <= __last; } +{ + return + __valid_range_aux(__first, __last, std::input_iterator_tag{}) ...^^^ here... + && __first <= __last; +} /** We have iterators, so figure out what kind of iterators they are * to see if we can check the range ahead of time. @@ -167,6 +194,9 @@ namespace __gnu_debug typename _Distance_traits<_InputIterator>::__type& __dist, std::__false_type) { + if (!__valid_range_aux(__first, __last, std::input_iterator_tag{})) ...and ^^^ here are not allowed pre C++11. Replacing those with std::input_iterator_tag() should fix it. + return false; + __dist = __get_distance(__first, __last); switch (__dist.second) {
Re: [PATCH][_GLIBCXX_DEBUG] Improve valid_range check
On 22/11/19 18:38 +0100, François Dumont wrote: Hi I noticed that we are not checking that iterators are not singular in valid_range. Moreover __check_singular signature for pointers is not intercepting all kind of pointers in terms of qualification. I'd like to commit it next week but considering we are in stage 3 I need proper acceptance. * include/debug/functions.h: Remove include. (__check_singular_aux, __check_singular): Move... * include/debug/helper_functions.h: (__check_singular_aux, __check_singular): ...here. (__valid_range_aux): Adapt to use latter. * testsuite/25_algorithms/copy/debug/2_neg.cc: New. Tested under Linux x86_64 normal and debug modes. OK for trunk, thanks.