Message ID | 368143e7-e151-9e37-f055-065044a57e7a@gmail.com |
---|---|
State | New |
Headers | show |
Series | Help compiler detect invalid code | expand |
I already realized that previous patch will be too controversial to be accepted. In this new version I just implement a real memmove in __memmove so that in copy_backward there is no need for a shortcut to a more defensive code. I'll see if in Debug mode I can do something. François On 9/19/19 10:27 PM, François Dumont wrote: > Hi > > I start working on making recently added constexpr tests to work > in Debug mode. > > It appears that the compiler is able to detect code mistakes > pretty well as long we don't try to hide the code intention with a > defensive approach. This is why I'd like to propose to replace '__n > > 0' conditions with '__n != 0'. > > The result is demonstrated by the constexpr_neg.cc tests. What do > you think ? > > * include/bits/stl_algobase.h (__memmove): Return _Tp*. > (__memmove): Loop as long as __n is not 0. > (__copy_move<>::__copy_m): Likewise. > (__copy_move_backward<>::__copy_move_b): Likewise. > * testsuite/25_algorithms/copy/constexpr.cc: Add check on copied > values. > * testsuite/25_algorithms/copy_backward/constexpr.cc: Likewise. > * testsuite/25_algorithms/copy/constexpr_neg.cc: New. > * testsuite/25_algorithms/copy_backward/constexpr.cc: New. > > I'll submit the patch to fix Debug mode depending on the decision > for this one. > > François >
Some more tests have revealed a small problem in is_sorted test. It was revealed by the Debug mode but not in a clean ways so for the moment I prefer to fix it and I'll add a _neg test when Debug is able to report it correctly. I've also added a _neg test for equal which doesn't need Debug mode. OK to commit ? François On 9/20/19 7:08 AM, François Dumont wrote: > I already realized that previous patch will be too controversial to be > accepted. > > In this new version I just implement a real memmove in __memmove so > that in copy_backward there is no need for a shortcut to a more > defensive code. > > I'll see if in Debug mode I can do something. > > François > > > On 9/19/19 10:27 PM, François Dumont wrote: >> Hi >> >> I start working on making recently added constexpr tests to work >> in Debug mode. >> >> It appears that the compiler is able to detect code mistakes >> pretty well as long we don't try to hide the code intention with a >> defensive approach. This is why I'd like to propose to replace '__n > >> 0' conditions with '__n != 0'. >> >> The result is demonstrated by the constexpr_neg.cc tests. What do >> you think ? >> >> * include/bits/stl_algobase.h (__memmove): Return _Tp*. >> (__memmove): Loop as long as __n is not 0. >> (__copy_move<>::__copy_m): Likewise. >> (__copy_move_backward<>::__copy_move_b): Likewise. >> * testsuite/25_algorithms/copy/constexpr.cc: Add check on copied >> values. >> * testsuite/25_algorithms/copy_backward/constexpr.cc: Likewise. >> * testsuite/25_algorithms/copy/constexpr_neg.cc: New. >> * testsuite/25_algorithms/copy_backward/constexpr.cc: New. >> >> I'll submit the patch to fix Debug mode depending on the decision >> for this one. >> >> François >> >
On 20/09/19 07:08 +0200, François Dumont wrote: >I already realized that previous patch will be too controversial to be >accepted. > >In this new version I just implement a real memmove in __memmove so A real memmove doesn't just work backwards, it needs to detect any overlaps and work forwards *or* backwards, as needed. I think your change will break this case: #include <algorithm> constexpr int f(int i, int j, int k) { int arr[5] = { 0, 0, i, j, k }; std::move(arr+2, arr+5, arr); return arr[0] + arr[1] + arr[2]; } static_assert( f(1, 2, 3) == 6 ); This is valid because std::move only requires that the result iterator is not in the input range, but it's OK for the two ranges to overlap. I haven't tested it, but I think with your change the array will end up containing {3, 2, 3, 2, 3} instead of {1, 2, 3, 2, 3}.
On 19/09/19 22:27 +0200, François Dumont wrote: >Hi > > I start working on making recently added constexpr tests to work >in Debug mode. The attached patch seems to be necessary for that, right?
On 9/27/19 2:11 PM, Jonathan Wakely wrote: > On 19/09/19 22:27 +0200, François Dumont wrote: >> Hi >> >> I start working on making recently added constexpr tests to work >> in Debug mode. > > The attached patch seems to be necessary for that, right? > > On my side I had done this, almost the same. For the moment there is a FIXME in macros.h to find out how to generate a nice compilation error when the condition is not meant. static_assert can't be called in this context, too bad. I also try to define a function with a __attribute__((__error__("because"))) attribute. But when I make it constexpr gcc complains about missing definition. When I provide a definition gcc complains that this attribute must be on a declaration. And when I split declaration and definition gcc does not produce the expected compilation error. Unless you have the solution I consider that we need help from the front-end. For the moment if Debug mode finds a problem it will be reported as _M_error function not being constexpr ! François
On 27/09/19 18:24 +0200, François Dumont wrote: >On 9/27/19 2:11 PM, Jonathan Wakely wrote: >>On 19/09/19 22:27 +0200, François Dumont wrote: >>>Hi >>> >>> I start working on making recently added constexpr tests to >>>work in Debug mode. >> >>The attached patch seems to be necessary for that, right? >> >> >On my side I had done this, almost the same. > >For the moment there is a FIXME in macros.h to find out how to >generate a nice compilation error when the condition is not meant. > >static_assert can't be called in this context, too bad. > >I also try to define a function with a >__attribute__((__error__("because"))) attribute. But when I make it >constexpr gcc complains about missing definition. When I provide a >definition gcc complains that this attribute must be on a declaration. >And when I split declaration and definition gcc does not produce the >expected compilation error. Yes, I've tried similar things without success. >Unless you have the solution I consider that we need help from the >front-end. > >For the moment if Debug mode finds a problem it will be reported as >_M_error function not being constexpr ! A reasonable workaround is to do: #ifdef _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED if (__builtin_is_constant_evaluated()) asm("Debug Mode assertion failed"); else #endif if (!(Cond)) __gnu_debug::_Error_formatter::... The builtin is available even for C++98, whereas std::is_constant_evaluated() is only available for C++20. This produces errors that include lines like: asm.cc:12:17: in ‘constexpr’ expansion of ‘f(-1)’ asm.cc:4:7: error: inline assembly is not a constant expression 4 | asm("debug mode assertion failed"); | ^~~ asm.cc:8:3: note: in expansion of macro ‘CHECK’ 8 | _GLIBCXX_ASSERT(i > 0); | ^~~~~ asm.cc:4:7: note: only unevaluated inline assembly is allowed in a ‘constexpr’ function in C++2a 4 | asm("debug mode assertion failed"); | ^~~ asm.cc:8:3: note: in expansion of macro ‘CHECK’ 8 | CHECK(i > 0); | ^~~~~ It's not ideal, but it does show the failed condition and the text "debug mode assertion failed" (or whatever message you choose to use there).
On 9/27/19 1:24 PM, Jonathan Wakely wrote: > On 20/09/19 07:08 +0200, François Dumont wrote: >> I already realized that previous patch will be too controversial to >> be accepted. >> >> In this new version I just implement a real memmove in __memmove so > > A real memmove doesn't just work backwards, it needs to detect any > overlaps and work forwards *or* backwards, as needed. ok, good to know, I understand now why using __builtin_memcopy didn't show any performance enhancement when I tested it ! > > I think your change will break this case: > > #include <algorithm> > > constexpr int f(int i, int j, int k) > { > int arr[5] = { 0, 0, i, j, k }; > std::move(arr+2, arr+5, arr); > return arr[0] + arr[1] + arr[2]; > } > > static_assert( f(1, 2, 3) == 6 ); > > This is valid because std::move only requires that the result iterator > is not in the input range, but it's OK for the two ranges to overlap. > > I haven't tested it, but I think with your change the array will end > up containing {3, 2, 3, 2, 3} instead of {1, 2, 3, 2, 3}. > Indeed, I've added a std::move constexpr test in this new proposal which demonstrate that. C++ Standard clearly states that [copy|move]_backward is done backward. So in this new proposal I propose to add a __memcopy used in copy/move and keep __memmove for *_backward algos. Both are using __builtin_memmove as before. * include/bits/stl_algobase.h (__memmove): Return void, loop as long as __n != 0. (__memcopy): New. (__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m): Adapt to use latter. (__copy_move_backward_a): Remove std::is_constant_evaluated block. * testsuite/25_algorithms/copy/constexpr.cc (test): Add check on copied values. * testsuite/25_algorithms/copy_backward/constexpr.cc (test): Likewise and rename in test1. (test2): New. * testsuite/25_algorithms/copy/constexpr_neg.cc: New. * testsuite/25_algorithms/copy_backward/constexpr.cc: New. * testsuite/25_algorithms/equal/constexpr_neg.cc: New. * testsuite/25_algorithms/move/constexpr.cc: New. * testsuite/25_algorithms/move/constexpr_neg.cc: New. Tested under Linux x86_64. Ok to commit ? François
On 01/10/19 22:05 +0200, François Dumont wrote: >On 9/27/19 1:24 PM, Jonathan Wakely wrote: >>On 20/09/19 07:08 +0200, François Dumont wrote: >>>I already realized that previous patch will be too controversial >>>to be accepted. >>> >>>In this new version I just implement a real memmove in __memmove >>>so >> >>A real memmove doesn't just work backwards, it needs to detect any >>overlaps and work forwards *or* backwards, as needed. >ok, good to know, I understand now why using __builtin_memcopy didn't >show any performance enhancement when I tested it ! >> >>I think your change will break this case: >> >>#include <algorithm> >> >>constexpr int f(int i, int j, int k) >>{ >> int arr[5] = { 0, 0, i, j, k }; >> std::move(arr+2, arr+5, arr); >> return arr[0] + arr[1] + arr[2]; >>} >> >>static_assert( f(1, 2, 3) == 6 ); >> >>This is valid because std::move only requires that the result iterator >>is not in the input range, but it's OK for the two ranges to overlap. >> >>I haven't tested it, but I think with your change the array will end >>up containing {3, 2, 3, 2, 3} instead of {1, 2, 3, 2, 3}. >> >Indeed, I've added a std::move constexpr test in this new proposal >which demonstrate that. > >C++ Standard clearly states that [copy|move]_backward is done >backward. So in this new proposal I propose to add a __memcopy used in >copy/move and keep __memmove for *_backward algos. Both are using >__builtin_memmove as before. Then they *really* need better names now (__memmove was already a bad name, but now it's terrible). If the difference is that one goes forwards and one goes backwards, the names should reflect that. I'll review it properly tomorrow.
Here is a version with __detail::__copy and __detail::__copy_backward. I prefered to introduce the __detail namespace cause __copy is quite a common name so putting it in __detail namespace will I hope clarify that it is for internal usage only. I even hesitated to put more details into this namespace, maybe for another patch later. * include/bits/stl_algobase.h (__memmove): Replace by... (__detail::__copy) ...that. Return void, loop as long as __n != 0. (__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m): Adapt to use latter. (__detail::__copy_backward): New. (__copy_move_backward<_IsMove, true, std::random_access_iterator_tag>::__copy_m): Adapt to use latter. (__copy_move_backward_a): Remove std::is_constant_evaluated block. * testsuite/25_algorithms/copy/constexpr.cc (test): Add check on copied values. * testsuite/25_algorithms/copy_backward/constexpr.cc (test): Likewise and rename in test1. (test2): New. * testsuite/25_algorithms/copy/constexpr_neg.cc: New. * testsuite/25_algorithms/copy_backward/constexpr.cc: New. * testsuite/25_algorithms/equal/constexpr_neg.cc: New. * testsuite/25_algorithms/move/constexpr.cc: New. * testsuite/25_algorithms/move/constexpr_neg.cc: New. François On 10/10/19 10:03 PM, Jonathan Wakely wrote: > On 01/10/19 22:05 +0200, François Dumont wrote: >> On 9/27/19 1:24 PM, Jonathan Wakely wrote: >>> On 20/09/19 07:08 +0200, François Dumont wrote: >>>> I already realized that previous patch will be too controversial to >>>> be accepted. >>>> >>>> In this new version I just implement a real memmove in __memmove so >>> >>> A real memmove doesn't just work backwards, it needs to detect any >>> overlaps and work forwards *or* backwards, as needed. >> ok, good to know, I understand now why using __builtin_memcopy didn't >> show any performance enhancement when I tested it ! >>> >>> I think your change will break this case: >>> >>> #include <algorithm> >>> >>> constexpr int f(int i, int j, int k) >>> { >>> int arr[5] = { 0, 0, i, j, k }; >>> std::move(arr+2, arr+5, arr); >>> return arr[0] + arr[1] + arr[2]; >>> } >>> >>> static_assert( f(1, 2, 3) == 6 ); >>> >>> This is valid because std::move only requires that the result iterator >>> is not in the input range, but it's OK for the two ranges to overlap. >>> >>> I haven't tested it, but I think with your change the array will end >>> up containing {3, 2, 3, 2, 3} instead of {1, 2, 3, 2, 3}. >>> >> Indeed, I've added a std::move constexpr test in this new proposal >> which demonstrate that. >> >> C++ Standard clearly states that [copy|move]_backward is done >> backward. So in this new proposal I propose to add a __memcopy used >> in copy/move and keep __memmove for *_backward algos. Both are using >> __builtin_memmove as before. > > Then they *really* need better names now (__memmove was already a bad > name, but now it's terrible). If the difference is that one goes > forwards and one goes backwards, the names should reflect that. > > I'll review it properly tomorrow. > >
diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h index 4eba053ac75..33593197b4f 100644 --- a/libstdc++-v3/include/bits/stl_algobase.h +++ b/libstdc++-v3/include/bits/stl_algobase.h @@ -83,13 +83,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION */ template<bool _IsMove, typename _Tp> _GLIBCXX14_CONSTEXPR - inline void* - __memmove(_Tp* __dst, const _Tp* __src, size_t __num) + inline _Tp* + __memmove(_Tp* __dst, const _Tp* __src, ptrdiff_t __num) { #ifdef __cpp_lib_is_constant_evaluated if (std::is_constant_evaluated()) { - for(; __num > 0; --__num) + for (; __num != 0; --__num) { if constexpr (_IsMove) *__dst = std::move(*__src); @@ -100,10 +100,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } return __dst; } - else #endif - return __builtin_memmove(__dst, __src, sizeof(_Tp) * __num); - return __dst; + return static_cast<_Tp*>( + __builtin_memmove(__dst, __src, sizeof(_Tp) * __num)); } /* @@ -398,7 +397,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __copy_m(_II __first, _II __last, _OI __result) { typedef typename iterator_traits<_II>::difference_type _Distance; - for(_Distance __n = __last - __first; __n > 0; --__n) + for (_Distance __n = __last - __first; __n != 0; --__n) { *__result = *__first; ++__first; @@ -418,7 +417,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __copy_m(_II __first, _II __last, _OI __result) { typedef typename iterator_traits<_II>::difference_type _Distance; - for(_Distance __n = __last - __first; __n > 0; --__n) + for (_Distance __n = __last - __first; __n != 0; --__n) { *__result = std::move(*__first); ++__first; @@ -446,8 +445,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #endif const ptrdiff_t _Num = __last - __first; if (_Num) - std::__memmove<_IsMove>(__result, __first, _Num); - return __result + _Num; + return std::__memmove<_IsMove>(__result, __first, _Num); + return __result; } }; @@ -671,7 +670,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER { typename iterator_traits<_BI1>::difference_type __n = __last - __first; - for (; __n > 0; --__n) + for (; __n != 0; --__n) *--__result = *--__last; return __result; } @@ -688,7 +687,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER { typename iterator_traits<_BI1>::difference_type __n = __last - __first; - for (; __n > 0; --__n) + for (; __n != 0; --__n) *--__result = std::move(*--__last); return __result; } diff --git a/libstdc++-v3/testsuite/25_algorithms/copy/constexpr.cc b/libstdc++-v3/testsuite/25_algorithms/copy/constexpr.cc index 67910b8773e..a0460603496 100644 --- a/libstdc++-v3/testsuite/25_algorithms/copy/constexpr.cc +++ b/libstdc++-v3/testsuite/25_algorithms/copy/constexpr.cc @@ -24,12 +24,12 @@ constexpr bool test() { - constexpr std::array<int, 12> ca0{{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}}; + constexpr std::array<int, 12> ca0{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12}}; std::array<int, 12> ma0{{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}}; const auto out6 = std::copy(ca0.begin(), ca0.begin() + 8, ma0.begin() + 2); - return out6 == ma0.begin() + 10; + return out6 == ma0.begin() + 10 && *(ma0.begin() + 2) == 1 && *out6 == 0; } static_assert(test()); diff --git a/libstdc++-v3/testsuite/25_algorithms/copy/constexpr_neg.cc b/libstdc++-v3/testsuite/25_algorithms/copy/constexpr_neg.cc new file mode 100644 index 00000000000..34e20be97eb --- /dev/null +++ b/libstdc++-v3/testsuite/25_algorithms/copy/constexpr_neg.cc @@ -0,0 +1,50 @@ +// Copyright (C) 2019 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +// { dg-options "-std=gnu++2a" } +// { dg-do compile { target c++2a xfail *-*-* } } + +#include <algorithm> +#include <array> + +constexpr bool +test1() +{ + constexpr std::array<int, 12> ca0{{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}}; + std::array<int, 12> ma0{{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}}; + + const auto out6 = std::copy(ca0.begin() + 8, ca0.begin(), ma0.begin() + 2); + + return out6 == ma0.begin() + 10; +} + +static_assert(test1()); // { dg-error "outside the bounds" } + +constexpr bool +test2() +{ + std::array<int, 12> ma0{{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}}; + const auto out6 = std::copy(ma0.begin(), ma0.begin() + 8, ma0.begin() + 2); + + // Check what should be the result if the range didn't overlap. + return out6 == ma0.begin() + 10 && *(ma0.begin() + 9) == 7; +} + +static_assert(test2()); // { dg-error "static assertion failed" } + +// { dg-prune-output "non-constant condition" } +// { dg-prune-output "in 'constexpr'" } diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_backward/constexpr.cc b/libstdc++-v3/testsuite/25_algorithms/copy_backward/constexpr.cc index ed7487905a8..c0e1a747832 100644 --- a/libstdc++-v3/testsuite/25_algorithms/copy_backward/constexpr.cc +++ b/libstdc++-v3/testsuite/25_algorithms/copy_backward/constexpr.cc @@ -22,15 +22,27 @@ #include <array> constexpr bool -test() +test1() { - constexpr std::array<int, 12> ca0{{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}}; + constexpr std::array<int, 12> ca0{{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12}}; std::array<int, 12> ma0{{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}}; const auto out7 = std::copy_backward(ca0.begin(), ca0.begin() + 8, ma0.begin() + 10); - return true; + return out7 == ma0.begin() + 2 && *out7 == 1 && *(ma0.begin() + 10) == 0; } -static_assert(test()); +static_assert(test1()); + +constexpr bool +test2() +{ + std::array<int, 12> ma0{{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}}; + const auto out7 = std::copy_backward(ma0.begin(), ma0.begin() + 8, + ma0.begin() + 10); + + return out7 == ma0.begin() + 2 && *out7 == 0 && *(ma0.begin() + 9) == 7; +} + +static_assert(test2()); diff --git a/libstdc++-v3/testsuite/25_algorithms/copy_backward/constexpr_neg.cc b/libstdc++-v3/testsuite/25_algorithms/copy_backward/constexpr_neg.cc new file mode 100644 index 00000000000..3dc595d239f --- /dev/null +++ b/libstdc++-v3/testsuite/25_algorithms/copy_backward/constexpr_neg.cc @@ -0,0 +1,39 @@ +// Copyright (C) 2019 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +// { dg-options "-std=gnu++2a" } +// { dg-do compile { target c++2a xfail *-*-* } } + +#include <algorithm> +#include <array> + +constexpr bool +test() +{ + constexpr std::array<int, 12> ca0{{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}}; + std::array<int, 12> ma0{{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}}; + + const auto out7 = std::copy_backward(ca0.begin() + 8, ca0.begin(), + ma0.begin() + 10); + + return out7 == ma0.begin() + 2; +} + +static_assert(test()); // { dg-error "outside the bounds" } + +// { dg-prune-output "non-constant condition" } +// { dg-prune-output "in 'constexpr'" }