public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light
@ 2021-05-27 17:37 François Dumont
  2021-05-31 17:17 ` François Dumont
  2021-06-03 12:31 ` Jonathan Wakely
  0 siblings, 2 replies; 7+ messages in thread
From: François Dumont @ 2021-05-27 17:37 UTC (permalink / raw)
  To: libstdc++; +Cc: gcc-patches

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

We have been talking for a long time of a debug mode with less impact on 
performances.

I propose to simply use the existing _GLIBCXX_ASSERTIONS macro.

     libstdc++: [_GLIBCXX_ASSERTIONS] Activate basic debug checks

     Use _GLIBCXX_ASSERTIONS as a _GLIBCXX_DEBUG light mode. When 
defined it activates
     all _GLIBCXX_DEBUG checks but skipping those requiring to loop 
through the iterator
     range unless in case of constexpr.

     libstdc++-v3/ChangeLog:

             * include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define debug 
macros non-empty.
             * include/debug/helper_functions.h: Cleanup comment about 
removed _Iter_base.
             * include/debug/functions.h (__skip_debug_runtime_check): 
New, returns false if
             _GLIBCXX_DEBUG is defined or if constant evaluated.
             (__check_sorted, __check_partitioned_lower, 
__check_partitioned_upper): Use latter.

Tested under Linux x64.

Ok to commit ?

François


[-- Attachment #2: assertion_mode.patch --]
[-- Type: text/x-patch, Size: 4206 bytes --]

diff --git a/libstdc++-v3/include/debug/debug.h b/libstdc++-v3/include/debug/debug.h
index 116f2f023e2..2e6ce1c8a93 100644
--- a/libstdc++-v3/include/debug/debug.h
+++ b/libstdc++-v3/include/debug/debug.h
@@ -61,7 +61,7 @@ namespace __gnu_debug
     struct _Safe_iterator;
 }
 
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
 
 # define __glibcxx_requires_cond(_Cond,_Msg)
 # define __glibcxx_requires_valid_range(_First,_Last)
diff --git a/libstdc++-v3/include/debug/functions.h b/libstdc++-v3/include/debug/functions.h
index 6cac11f2abd..ee0eb877568 100644
--- a/libstdc++-v3/include/debug/functions.h
+++ b/libstdc++-v3/include/debug/functions.h
@@ -48,6 +48,25 @@ namespace __gnu_debug
   template<typename _Sequence>
     struct _Is_contiguous_sequence : std::__false_type { };
 
+  _GLIBCXX20_CONSTEXPR
+  inline bool
+  __skip_debug_runtime_check()
+  {
+    // We could be here while only _GLIBCXX_ASSERTIONS has been defined.
+    // In this case we skip expensive runtime checks, constexpr will still
+    // be checked.
+    return
+#ifndef _GLIBCXX_DEBUG
+# if _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED
+      !__builtin_is_constant_evaluated();
+# else
+      true;
+# endif
+#else
+      false;
+#endif
+  }
+
   /* Checks that [first, last) is a valid range, and then returns
    * __first. This routine is useful when we can't use a separate
    * assertion statement because, e.g., we are in a constructor.
@@ -260,8 +279,9 @@ namespace __gnu_debug
     inline bool
     __check_sorted(const _InputIterator& __first, const _InputIterator& __last)
     {
-      return __check_sorted_aux(__first, __last,
-				std::__iterator_category(__first));
+      return __skip_debug_runtime_check()
+	|| __check_sorted_aux(__first, __last,
+			      std::__iterator_category(__first));
     }
 
   template<typename _InputIterator, typename _Predicate>
@@ -270,8 +290,9 @@ namespace __gnu_debug
     __check_sorted(const _InputIterator& __first, const _InputIterator& __last,
                    _Predicate __pred)
     {
-      return __check_sorted_aux(__first, __last, __pred,
-				std::__iterator_category(__first));
+      return __skip_debug_runtime_check()
+	|| __check_sorted_aux(__first, __last, __pred,
+			      std::__iterator_category(__first));
     }
 
   template<typename _InputIterator>
@@ -351,6 +372,9 @@ namespace __gnu_debug
     __check_partitioned_lower(_ForwardIterator __first,
 			      _ForwardIterator __last, const _Tp& __value)
     {
+      if (__skip_debug_runtime_check())
+	return true;
+
       while (__first != __last && *__first < __value)
 	++__first;
       if (__first != __last)
@@ -368,6 +392,9 @@ namespace __gnu_debug
     __check_partitioned_upper(_ForwardIterator __first,
 			      _ForwardIterator __last, const _Tp& __value)
     {
+      if (__skip_debug_runtime_check())
+	return true;
+
       while (__first != __last && !(__value < *__first))
 	++__first;
       if (__first != __last)
@@ -387,6 +414,9 @@ namespace __gnu_debug
 			      _ForwardIterator __last, const _Tp& __value,
 			      _Pred __pred)
     {
+      if (__skip_debug_runtime_check())
+	return true;
+
       while (__first != __last && bool(__pred(*__first, __value)))
 	++__first;
       if (__first != __last)
@@ -405,6 +435,9 @@ namespace __gnu_debug
 			      _ForwardIterator __last, const _Tp& __value,
 			      _Pred __pred)
     {
+      if (__skip_debug_runtime_check())
+	return true;
+
       while (__first != __last && !bool(__pred(__value, *__first)))
 	++__first;
       if (__first != __last)
diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h
index c0144ced979..587eba2f3e5 100644
--- a/libstdc++-v3/include/debug/helper_functions.h
+++ b/libstdc++-v3/include/debug/helper_functions.h
@@ -30,8 +30,8 @@
 #define _GLIBCXX_DEBUG_HELPER_FUNCTIONS_H 1
 
 #include <bits/move.h>				// for __addressof
-#include <bits/stl_iterator_base_types.h>	// for iterator_traits,
-						// categories and _Iter_base
+#include <bits/stl_iterator_base_types.h>	// for iterator_traits and
+						// categories
 #include <bits/cpp_type_traits.h>		// for __is_integer
 
 #include <bits/stl_pair.h>			// for pair


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light
  2021-05-27 17:37 [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light François Dumont
@ 2021-05-31 17:17 ` François Dumont
  2021-06-03 12:31 ` Jonathan Wakely
  1 sibling, 0 replies; 7+ messages in thread
From: François Dumont @ 2021-05-31 17:17 UTC (permalink / raw)
  To: libstdc++; +Cc: gcc-patches

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

Completing the tests revealed that this patch was missing a small change 
in include/bits/stl_iterator.h. Here is the updated patch.

     libstdc++: [_GLIBCXX_ASSERTIONS] Activate basic debug checks

     Use _GLIBCXX_ASSERTIONS as a _GLIBCXX_DEBUG light mode. When 
defined it activates
     all _GLIBCXX_DEBUG checks but skipping those requiring to loop 
through the iterator
     range unless in case of constexpr.

     libstdc++-v3/ChangeLog:

             * include/bits/stl_iterator.h [_GLIBCXX_ASSERTIONS]: 
Include <debug/stl_iterator.h>.
             * include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define debug 
macros non-empty.
             * include/debug/helper_functions.h: Cleanup comment about 
removed _Iter_base.
             * include/debug/functions.h (__skip_debug_runtime_check): 
New, returns false if
             _GLIBCXX_DEBUG is defined or if constant evaluated.
             (__check_sorted, __check_partitioned_lower, 
__check_partitioned_upper): Use latter.

Ok to commit ?

François


On 27/05/21 7:37 pm, François Dumont wrote:
> We have been talking for a long time of a debug mode with less impact 
> on performances.
>
> I propose to simply use the existing _GLIBCXX_ASSERTIONS macro.
>
>     libstdc++: [_GLIBCXX_ASSERTIONS] Activate basic debug checks
>
>     Use _GLIBCXX_ASSERTIONS as a _GLIBCXX_DEBUG light mode. When 
> defined it activates
>     all _GLIBCXX_DEBUG checks but skipping those requiring to loop 
> through the iterator
>     range unless in case of constexpr.
>
>     libstdc++-v3/ChangeLog:
>
>             * include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define 
> debug macros non-empty.
>             * include/debug/helper_functions.h: Cleanup comment about 
> removed _Iter_base.
>             * include/debug/functions.h (__skip_debug_runtime_check): 
> New, returns false if
>             _GLIBCXX_DEBUG is defined or if constant evaluated.
>             (__check_sorted, __check_partitioned_lower, 
> __check_partitioned_upper): Use latter.
>
> Tested under Linux x64.
>
> Ok to commit ?
>
> François
>


[-- Attachment #2: assertion_mode.patch --]
[-- Type: text/x-patch, Size: 4634 bytes --]

diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
index 8768624b7d1..015b5da1d18 100644
--- a/libstdc++-v3/include/bits/stl_iterator.h
+++ b/libstdc++-v3/include/bits/stl_iterator.h
@@ -2385,7 +2385,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace
 
-#ifdef _GLIBCXX_DEBUG
+#ifdef _GLIBCXX_ASSERTIONS
 # include <debug/stl_iterator.h>
 #endif
 
diff --git a/libstdc++-v3/include/debug/debug.h b/libstdc++-v3/include/debug/debug.h
index 116f2f023e2..2e6ce1c8a93 100644
--- a/libstdc++-v3/include/debug/debug.h
+++ b/libstdc++-v3/include/debug/debug.h
@@ -61,7 +61,7 @@ namespace __gnu_debug
     struct _Safe_iterator;
 }
 
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
 
 # define __glibcxx_requires_cond(_Cond,_Msg)
 # define __glibcxx_requires_valid_range(_First,_Last)
diff --git a/libstdc++-v3/include/debug/functions.h b/libstdc++-v3/include/debug/functions.h
index 6cac11f2abd..ee0eb877568 100644
--- a/libstdc++-v3/include/debug/functions.h
+++ b/libstdc++-v3/include/debug/functions.h
@@ -48,6 +48,25 @@ namespace __gnu_debug
   template<typename _Sequence>
     struct _Is_contiguous_sequence : std::__false_type { };
 
+  _GLIBCXX20_CONSTEXPR
+  inline bool
+  __skip_debug_runtime_check()
+  {
+    // We could be here while only _GLIBCXX_ASSERTIONS has been defined.
+    // In this case we skip expensive runtime checks, constexpr will still
+    // be checked.
+    return
+#ifndef _GLIBCXX_DEBUG
+# if _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED
+      !__builtin_is_constant_evaluated();
+# else
+      true;
+# endif
+#else
+      false;
+#endif
+  }
+
   /* Checks that [first, last) is a valid range, and then returns
    * __first. This routine is useful when we can't use a separate
    * assertion statement because, e.g., we are in a constructor.
@@ -260,8 +279,9 @@ namespace __gnu_debug
     inline bool
     __check_sorted(const _InputIterator& __first, const _InputIterator& __last)
     {
-      return __check_sorted_aux(__first, __last,
-				std::__iterator_category(__first));
+      return __skip_debug_runtime_check()
+	|| __check_sorted_aux(__first, __last,
+			      std::__iterator_category(__first));
     }
 
   template<typename _InputIterator, typename _Predicate>
@@ -270,8 +290,9 @@ namespace __gnu_debug
     __check_sorted(const _InputIterator& __first, const _InputIterator& __last,
                    _Predicate __pred)
     {
-      return __check_sorted_aux(__first, __last, __pred,
-				std::__iterator_category(__first));
+      return __skip_debug_runtime_check()
+	|| __check_sorted_aux(__first, __last, __pred,
+			      std::__iterator_category(__first));
     }
 
   template<typename _InputIterator>
@@ -351,6 +372,9 @@ namespace __gnu_debug
     __check_partitioned_lower(_ForwardIterator __first,
 			      _ForwardIterator __last, const _Tp& __value)
     {
+      if (__skip_debug_runtime_check())
+	return true;
+
       while (__first != __last && *__first < __value)
 	++__first;
       if (__first != __last)
@@ -368,6 +392,9 @@ namespace __gnu_debug
     __check_partitioned_upper(_ForwardIterator __first,
 			      _ForwardIterator __last, const _Tp& __value)
     {
+      if (__skip_debug_runtime_check())
+	return true;
+
       while (__first != __last && !(__value < *__first))
 	++__first;
       if (__first != __last)
@@ -387,6 +414,9 @@ namespace __gnu_debug
 			      _ForwardIterator __last, const _Tp& __value,
 			      _Pred __pred)
     {
+      if (__skip_debug_runtime_check())
+	return true;
+
       while (__first != __last && bool(__pred(*__first, __value)))
 	++__first;
       if (__first != __last)
@@ -405,6 +435,9 @@ namespace __gnu_debug
 			      _ForwardIterator __last, const _Tp& __value,
 			      _Pred __pred)
     {
+      if (__skip_debug_runtime_check())
+	return true;
+
       while (__first != __last && !bool(__pred(__value, *__first)))
 	++__first;
       if (__first != __last)
diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h
index c0144ced979..587eba2f3e5 100644
--- a/libstdc++-v3/include/debug/helper_functions.h
+++ b/libstdc++-v3/include/debug/helper_functions.h
@@ -30,8 +30,8 @@
 #define _GLIBCXX_DEBUG_HELPER_FUNCTIONS_H 1
 
 #include <bits/move.h>				// for __addressof
-#include <bits/stl_iterator_base_types.h>	// for iterator_traits,
-						// categories and _Iter_base
+#include <bits/stl_iterator_base_types.h>	// for iterator_traits and
+						// categories
 #include <bits/cpp_type_traits.h>		// for __is_integer
 
 #include <bits/stl_pair.h>			// for pair

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light
  2021-05-27 17:37 [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light François Dumont
  2021-05-31 17:17 ` François Dumont
@ 2021-06-03 12:31 ` Jonathan Wakely
  2021-06-07  4:25   ` François Dumont
  1 sibling, 1 reply; 7+ messages in thread
From: Jonathan Wakely @ 2021-06-03 12:31 UTC (permalink / raw)
  To: François Dumont; +Cc: libstdc++, gcc-patches

On 27/05/21 19:37 +0200, François Dumont via Libstdc++ wrote:
>We have been talking for a long time of a debug mode with less impact 
>on performances.

We already have it, that's what _GLIBCXX_ASSERTIONS already is :-)

>I propose to simply use the existing _GLIBCXX_ASSERTIONS macro.
>
>    libstdc++: [_GLIBCXX_ASSERTIONS] Activate basic debug checks
>
>    Use _GLIBCXX_ASSERTIONS as a _GLIBCXX_DEBUG light mode. When 
>defined it activates
>    all _GLIBCXX_DEBUG checks but skipping those requiring to loop 
>through the iterator
>    range unless in case of constexpr.
>
>    libstdc++-v3/ChangeLog:
>
>            * include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define 
>debug macros non-empty.
>            * include/debug/helper_functions.h: Cleanup comment about 
>removed _Iter_base.
>            * include/debug/functions.h (__skip_debug_runtime_check): 
>New, returns false if
>            _GLIBCXX_DEBUG is defined or if constant evaluated.
>            (__check_sorted, __check_partitioned_lower, 
>__check_partitioned_upper): Use latter.
>
>Tested under Linux x64.
>
>Ok to commit ?
>
>François
>

>diff --git a/libstdc++-v3/include/debug/debug.h b/libstdc++-v3/include/debug/debug.h
>index 116f2f023e2..2e6ce1c8a93 100644
>--- a/libstdc++-v3/include/debug/debug.h
>+++ b/libstdc++-v3/include/debug/debug.h
>@@ -61,7 +61,7 @@ namespace __gnu_debug
>     struct _Safe_iterator;
> }
> 
>-#ifndef _GLIBCXX_DEBUG
>+#ifndef _GLIBCXX_ASSERTIONS
> 
> # define __glibcxx_requires_cond(_Cond,_Msg)
> # define __glibcxx_requires_valid_range(_First,_Last)
>diff --git a/libstdc++-v3/include/debug/functions.h b/libstdc++-v3/include/debug/functions.h
>index 6cac11f2abd..ee0eb877568 100644
>--- a/libstdc++-v3/include/debug/functions.h
>+++ b/libstdc++-v3/include/debug/functions.h
>@@ -48,6 +48,25 @@ namespace __gnu_debug
>   template<typename _Sequence>
>     struct _Is_contiguous_sequence : std::__false_type { };
> 
>+  _GLIBCXX20_CONSTEXPR

Should this be simply _GLIBCXX_CONSTEXPR so that it can be constexpr
in C++14 mode too? Or are there are never any debug checks in
functions that are already constexpr in C++14 or C++17?

>+  inline bool
>+  __skip_debug_runtime_check()
>+  {
>+    // We could be here while only _GLIBCXX_ASSERTIONS has been defined.
>+    // In this case we skip expensive runtime checks, constexpr will still
>+    // be checked.
>+    return
>+#ifndef _GLIBCXX_DEBUG
>+# if _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED
>+      !__builtin_is_constant_evaluated();
>+# else
>+      true;
>+# endif
>+#else
>+      false;
>+#endif

I think this would be simpler without the nesting, and without the
preprocessor checks halfway through the return statement:

#ifdef _GLIBCXX_DEBUG
     return false;
#elif _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED
     return !__builtin_is_constant_evaluated();
#else
     return true;
#endif


>+  }
>+
>   /* Checks that [first, last) is a valid range, and then returns
>    * __first. This routine is useful when we can't use a separate
>    * assertion statement because, e.g., we are in a constructor.
>@@ -260,8 +279,9 @@ namespace __gnu_debug
>     inline bool
>     __check_sorted(const _InputIterator& __first, const _InputIterator& __last)
>     {
>-      return __check_sorted_aux(__first, __last,
>-				std::__iterator_category(__first));
>+      return __skip_debug_runtime_check()
>+	|| __check_sorted_aux(__first, __last,
>+			      std::__iterator_category(__first));

Currently this function is never called at all ifndef _GLIBCXX_DEBUG.
With this change, it's going to be present for _GLIBCXX_ASSERTIONS,
and if it isn't inlined it's going to explode the code size.

Some linux distros are already building the entire distro with
_GLIBCXX_ASSERTIONS so I think we need to be quite careful about this
kind of large change affecting every algo.

So maybe we shouldn't enable these checks via _GLIBCXX_ASSERTIONS, but
a new macro.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light
  2021-06-03 12:31 ` Jonathan Wakely
@ 2021-06-07  4:25   ` François Dumont
  2021-08-06 14:52     ` François Dumont
  0 siblings, 1 reply; 7+ messages in thread
From: François Dumont @ 2021-06-07  4:25 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches

On 03/06/21 2:31 pm, Jonathan Wakely wrote:
> On 27/05/21 19:37 +0200, François Dumont via Libstdc++ wrote:
>> We have been talking for a long time of a debug mode with less impact 
>> on performances.
>
> We already have it, that's what _GLIBCXX_ASSERTIONS already is :-)
>
>> I propose to simply use the existing _GLIBCXX_ASSERTIONS macro.
>>
>>     libstdc++: [_GLIBCXX_ASSERTIONS] Activate basic debug checks
>>
>>     Use _GLIBCXX_ASSERTIONS as a _GLIBCXX_DEBUG light mode. When 
>> defined it activates
>>     all _GLIBCXX_DEBUG checks but skipping those requiring to loop 
>> through the iterator
>>     range unless in case of constexpr.
>>
>>     libstdc++-v3/ChangeLog:
>>
>>             * include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define 
>> debug macros non-empty.
>>             * include/debug/helper_functions.h: Cleanup comment about 
>> removed _Iter_base.
>>             * include/debug/functions.h (__skip_debug_runtime_check): 
>> New, returns false if
>>             _GLIBCXX_DEBUG is defined or if constant evaluated.
>>             (__check_sorted, __check_partitioned_lower, 
>> __check_partitioned_upper): Use latter.
>>
>> Tested under Linux x64.
>>
>> Ok to commit ?
>>
>> François
>>
>
>> diff --git a/libstdc++-v3/include/debug/debug.h 
>> b/libstdc++-v3/include/debug/debug.h
>> index 116f2f023e2..2e6ce1c8a93 100644
>> --- a/libstdc++-v3/include/debug/debug.h
>> +++ b/libstdc++-v3/include/debug/debug.h
>> @@ -61,7 +61,7 @@ namespace __gnu_debug
>>     struct _Safe_iterator;
>> }
>>
>> -#ifndef _GLIBCXX_DEBUG
>> +#ifndef _GLIBCXX_ASSERTIONS
>>
>> # define __glibcxx_requires_cond(_Cond,_Msg)
>> # define __glibcxx_requires_valid_range(_First,_Last)
>> diff --git a/libstdc++-v3/include/debug/functions.h 
>> b/libstdc++-v3/include/debug/functions.h
>> index 6cac11f2abd..ee0eb877568 100644
>> --- a/libstdc++-v3/include/debug/functions.h
>> +++ b/libstdc++-v3/include/debug/functions.h
>> @@ -48,6 +48,25 @@ namespace __gnu_debug
>>   template<typename _Sequence>
>>     struct _Is_contiguous_sequence : std::__false_type { };
>>
>> +  _GLIBCXX20_CONSTEXPR
>
> Should this be simply _GLIBCXX_CONSTEXPR so that it can be constexpr
> in C++14 mode too? Or are there are never any debug checks in
> functions that are already constexpr in C++14 or C++17?
>
>> +  inline bool
>> +  __skip_debug_runtime_check()
>> +  {
>> +    // We could be here while only _GLIBCXX_ASSERTIONS has been 
>> defined.
>> +    // In this case we skip expensive runtime checks, constexpr will 
>> still
>> +    // be checked.
>> +    return
>> +#ifndef _GLIBCXX_DEBUG
>> +# if _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED
>> +      !__builtin_is_constant_evaluated();
>> +# else
>> +      true;
>> +# endif
>> +#else
>> +      false;
>> +#endif
>
> I think this would be simpler without the nesting, and without the
> preprocessor checks halfway through the return statement:
>
> #ifdef _GLIBCXX_DEBUG
>     return false;
> #elif _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED
>     return !__builtin_is_constant_evaluated();
> #else
>     return true;
> #endif
>
>
>> +  }
>> +
>>   /* Checks that [first, last) is a valid range, and then returns
>>    * __first. This routine is useful when we can't use a separate
>>    * assertion statement because, e.g., we are in a constructor.
>> @@ -260,8 +279,9 @@ namespace __gnu_debug
>>     inline bool
>>     __check_sorted(const _InputIterator& __first, const 
>> _InputIterator& __last)
>>     {
>> -      return __check_sorted_aux(__first, __last,
>> -                std::__iterator_category(__first));
>> +      return __skip_debug_runtime_check()
>> +    || __check_sorted_aux(__first, __last,
>> +                  std::__iterator_category(__first));
>
> Currently this function is never called at all ifndef _GLIBCXX_DEBUG.
> With this change, it's going to be present for _GLIBCXX_ASSERTIONS,
> and if it isn't inlined it's going to explode the code size.
>
> Some linux distros are already building the entire distro with
> _GLIBCXX_ASSERTIONS so I think we need to be quite careful about this
> kind of large change affecting every algo.
>
> So maybe we shouldn't enable these checks via _GLIBCXX_ASSERTIONS, but
> a new macro.
>
_GLIBCXX_DEBUG is already rarely used, so will be yet another mode.

So let's forget about all this, thanks.

François


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light
  2021-06-07  4:25   ` François Dumont
@ 2021-08-06 14:52     ` François Dumont
  2021-08-08 19:34       ` François Dumont
  0 siblings, 1 reply; 7+ messages in thread
From: François Dumont @ 2021-08-06 14:52 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches

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

On 07/06/21 6:25 am, François Dumont wrote:
> On 03/06/21 2:31 pm, Jonathan Wakely wrote:
>>
>>> +  }
>>> +
>>>   /* Checks that [first, last) is a valid range, and then returns
>>>    * __first. This routine is useful when we can't use a separate
>>>    * assertion statement because, e.g., we are in a constructor.
>>> @@ -260,8 +279,9 @@ namespace __gnu_debug
>>>     inline bool
>>>     __check_sorted(const _InputIterator& __first, const 
>>> _InputIterator& __last)
>>>     {
>>> -      return __check_sorted_aux(__first, __last,
>>> -                std::__iterator_category(__first));
>>> +      return __skip_debug_runtime_check()
>>> +    || __check_sorted_aux(__first, __last,
>>> +                  std::__iterator_category(__first));
>>
>> Currently this function is never called at all ifndef _GLIBCXX_DEBUG.
>> With this change, it's going to be present for _GLIBCXX_ASSERTIONS,
>> and if it isn't inlined it's going to explode the code size.
>>
>> Some linux distros are already building the entire distro with
>> _GLIBCXX_ASSERTIONS so I think we need to be quite careful about this
>> kind of large change affecting every algo.
>>
>> So maybe we shouldn't enable these checks via _GLIBCXX_ASSERTIONS, but
>> a new macro.
>>
> _GLIBCXX_DEBUG is already rarely used, so will be yet another mode.
>
> So let's forget about all this, thanks.
>
I eventually wonder if your feedback was limited to the use of 
__check_sorted and some other codes perhaps.

So here is another proposal which activate a small subset of the 
_GLIBCXX_DEBUG checks in _GLIBCXX_ASSERTIONS but with far less code.

First, the _Error_formatter is not used, the injected checks are simply 
using __glibcxx_assert.

Second I reduced the number of accitaved checks, mostly the __valid_range.

I also enhance the valid_range check for constexpr because sometimes the 
normal implementation is good enough to let the compiler diagnose a 
potential issue in this context. This is for example the case of the 
std::equal implementation whereas the std::copy implementation is too 
defensive.

     libstdc++: [_GLIBCXX_ASSERTIONS] Activate basic debug checks

     libstdc++-v3/ChangeLog:

             * include/bits/stl_algobase.h (equal): Use runtime-only 
_GLIBCXX_DEBUG check.
             * include/bits/stl_iterator.h [_GLIBCXX_ASSERTIONS]: 
Include <debug/stl_iterator.h>.
             * include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define debug 
macros non-empty. Most of
             the time do a simple valid_range check.
             * include/debug/helper_functions.h: Cleanup comment about 
removed _Iter_base.
             (__valid_range): Add __skip_if_constexpr parameter and skip 
check when in a constexpr
             context.
             * include/debug/macros.h (_GLIBCXX_DEBUG_VERIFY): Define as 
__glibcxx_assert when only
             _GLIBCXX_ASSERTIONS is defined.
             (__glibcxx_check_valid_range): Add _SkipIfConstexpr parameter.
             (__glibcxx_check_can_increment_range): Likewise.
             * testsuite/24_iterators/istream_iterator/1.cc (test01): 
Skip iterator increment when
             _GLIBCXX_ASSERTIONS is defined.
             * testsuite/25_algorithms/copy/constexpr_neg.cc: New test.
             * testsuite/25_algorithms/heap/1.cc: Skip operation 
complexity checks when _GLIBCXX_ASSERTIONS
             is defined.
             * 
testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_neg.cc: 
Fix dg-prune-output reason.
             * 
testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_pred_neg.cc: 
Likewise.
             * 
testsuite/25_algorithms/lower_bound/debug/constexpr_valid_range_neg.cc: 
Likewise.
             * 
testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_neg.cc: 
Likewise.
             * 
testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_pred_neg.cc: 
Likewise.
             * 
testsuite/25_algorithms/upper_bound/debug/constexpr_valid_range_neg.cc: 
Likewise.

The last fixes below are due to the recent changes to the 
__glibcxx_assert macro but it is close to the code I am changing so I 
prefer to fix those here.

Tested under Linux x86_64 w/o _GLIBCXX_ASSERTIONS.

Ok to commit ?

François


[-- Attachment #2: assertion_mode.patch --]
[-- Type: text/x-patch, Size: 22902 bytes --]

diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h
index d0c49628d7f..d45eec4339b 100644
--- a/libstdc++-v3/include/bits/stl_algobase.h
+++ b/libstdc++-v3/include/bits/stl_algobase.h
@@ -1551,7 +1551,7 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO
       __glibcxx_function_requires(_EqualOpConcept<
 	    typename iterator_traits<_II1>::value_type,
 	    typename iterator_traits<_II2>::value_type>)
-      __glibcxx_requires_can_increment_range(__first1, __last1, __first2);
+      __glibcxx_requires_can_increment_range_runtime(__first1, __last1, __first2);
 
       return std::__equal_aux(__first1, __last1, __first2);
     }
diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
index 3773d600b8f..37c348adfd3 100644
--- a/libstdc++-v3/include/bits/stl_iterator.h
+++ b/libstdc++-v3/include/bits/stl_iterator.h
@@ -2465,7 +2465,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace
 
-#ifdef _GLIBCXX_DEBUG
+#ifdef _GLIBCXX_ASSERTIONS
 # include <debug/stl_iterator.h>
 #endif
 
diff --git a/libstdc++-v3/include/debug/debug.h b/libstdc++-v3/include/debug/debug.h
index 116f2f023e2..d30ef0f25b2 100644
--- a/libstdc++-v3/include/debug/debug.h
+++ b/libstdc++-v3/include/debug/debug.h
@@ -61,12 +61,13 @@ namespace __gnu_debug
     struct _Safe_iterator;
 }
 
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
 
 # define __glibcxx_requires_cond(_Cond,_Msg)
 # define __glibcxx_requires_valid_range(_First,_Last)
 # define __glibcxx_requires_can_increment(_First,_Size)
 # define __glibcxx_requires_can_increment_range(_First1,_Last1,_First2)
+# define __glibcxx_requires_can_increment_range_runtime(_First1,_Last1,_First2)
 # define __glibcxx_requires_can_decrement_range(_First1,_Last1,_First2)
 # define __glibcxx_requires_sorted(_First,_Last)
 # define __glibcxx_requires_sorted_pred(_First,_Last,_Pred)
@@ -91,11 +92,19 @@ namespace __gnu_debug
 
 # define __glibcxx_requires_cond(_Cond,_Msg) _GLIBCXX_DEBUG_VERIFY(_Cond,_Msg)
 # define __glibcxx_requires_valid_range(_First,_Last)	\
-  __glibcxx_check_valid_range(_First,_Last)
+  __glibcxx_check_valid_range(_First,_Last,false)
 # define __glibcxx_requires_can_increment(_First,_Size)	\
   __glibcxx_check_can_increment(_First,_Size)
-# define __glibcxx_requires_can_increment_range(_First1,_Last1,_First2)	\
-  __glibcxx_check_can_increment_range(_First1,_Last1,_First2)
+# define __glibcxx_requires_irreflexive(_First,_Last)	\
+  __glibcxx_check_irreflexive(_First,_Last)
+# define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred)	\
+  __glibcxx_check_irreflexive_pred(_First,_Last,_Pred)
+
+#ifdef _GLIBCXX_DEBUG
+# define __glibcxx_requires_can_increment_range(_First1,_Last1,_First2) \
+  __glibcxx_check_can_increment_range(_First1,_Last1,_First2,false)
+# define __glibcxx_requires_can_increment_range_runtime(_First1,_Last1,_First2) \
+  __glibcxx_check_can_increment_range(_First1,_Last1,_First2,true)
 # define __glibcxx_requires_can_decrement_range(_First1,_Last1,_First2)	\
   __glibcxx_check_can_decrement_range(_First1,_Last1,_First2)
 # define __glibcxx_requires_sorted(_First,_Last)	\
@@ -121,16 +130,46 @@ namespace __gnu_debug
 # define __glibcxx_requires_string(_String) __glibcxx_check_string(_String)
 # define __glibcxx_requires_string_len(_String,_Len)	\
   __glibcxx_check_string_len(_String,_Len)
-# define __glibcxx_requires_irreflexive(_First,_Last)	\
-  __glibcxx_check_irreflexive(_First,_Last)
 # define __glibcxx_requires_irreflexive2(_First,_Last)	\
   __glibcxx_check_irreflexive2(_First,_Last)
-# define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred)	\
-  __glibcxx_check_irreflexive_pred(_First,_Last,_Pred)
 # define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred)	\
   __glibcxx_check_irreflexive_pred2(_First,_Last,_Pred)
 
 # include <debug/functions.h>
+#else /* _GLIBCXX_ASSERTIONS */
+# define __glibcxx_requires_can_increment_range(_First1,_Last1,_First2) \
+  __glibcxx_check_valid_range(_First1,_Last1,false)
+# define __glibcxx_requires_can_increment_range_runtime(_First1,_Last1,_First2) \
+  __glibcxx_check_valid_range(_First1,_Last1,true)
+# define __glibcxx_requires_can_decrement_range(_First1,_Last1,_First2)	\
+  __glibcxx_check_valid_range(_First1,_Last1,false)
+# define __glibcxx_requires_sorted(_First,_Last)			\
+  __glibcxx_check_valid_range(_First,_Last,false);			\
+  __glibcxx_check_irreflexive(_First,_Last)
+# define __glibcxx_requires_sorted_pred(_First,_Last,_Pred)		\
+  __glibcxx_check_valid_range(_First,_Last,false);			\
+  __glibcxx_check_irreflexive_pred(_First,_Last,_Pred)
+# define __glibcxx_requires_sorted_set(_First1,_Last1,_First2)		\
+  __glibcxx_check_valid_range(_First1,_Last1,false)
+# define __glibcxx_requires_sorted_set_pred(_First1,_Last1,_First2,_Pred) \
+  __glibcxx_check_valid_range(_First1,_Last1,false)
+# define __glibcxx_requires_partitioned_lower(_First,_Last,_Value)	\
+  __glibcxx_check_valid_range(_First,_Last,false)
+# define __glibcxx_requires_partitioned_upper(_First,_Last,_Value)	\
+  __glibcxx_check_valid_range(_First,_Last,false)
+# define __glibcxx_requires_partitioned_lower_pred(_First,_Last,_Value,_Pred) \
+  __glibcxx_check_valid_range(_First,_Last,false)
+# define __glibcxx_requires_partitioned_upper_pred(_First,_Last,_Value,_Pred) \
+  __glibcxx_check_valid_range(_First,_Last,false)
+# define __glibcxx_requires_heap(_First,_Last)
+# define __glibcxx_requires_heap_pred(_First,_Last,_Pred)
+# define __glibcxx_requires_string(_String)
+# define __glibcxx_requires_string_len(_String,_Len)
+# define __glibcxx_requires_irreflexive2(_First,_Last)
+# define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred)
+
+# include <debug/helper_functions.h>
+#endif /* _GLIBCXX_ASSERTIONS */
 
 #endif
 
diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h
index c0144ced979..fd7c42ba081 100644
--- a/libstdc++-v3/include/debug/helper_functions.h
+++ b/libstdc++-v3/include/debug/helper_functions.h
@@ -30,8 +30,8 @@
 #define _GLIBCXX_DEBUG_HELPER_FUNCTIONS_H 1
 
 #include <bits/move.h>				// for __addressof
-#include <bits/stl_iterator_base_types.h>	// for iterator_traits,
-						// categories and _Iter_base
+#include <bits/stl_iterator_base_types.h>	// for iterator_traits and
+						// categories
 #include <bits/cpp_type_traits.h>		// for __is_integer
 
 #include <bits/stl_pair.h>			// for pair
@@ -237,8 +237,13 @@ namespace __gnu_debug
     _GLIBCXX20_CONSTEXPR
     inline bool
     __valid_range(_InputIterator __first, _InputIterator __last,
-		  typename _Distance_traits<_InputIterator>::__type& __dist)
+		  typename _Distance_traits<_InputIterator>::__type& __dist,
+		  bool __skip_if_constexpr __attribute__ ((__unused__)))
     {
+#ifdef __cpp_lib_is_constant_evaluated
+      if (std::is_constant_evaluated() && __skip_if_constexpr)
+	return true;
+#endif
       typedef typename std::__is_integer<_InputIterator>::__type _Integral;
       return __valid_range_aux(__first, __last, __dist, _Integral());
     }
@@ -247,21 +252,28 @@ namespace __gnu_debug
     bool
     __valid_range(const _Safe_iterator<_Iterator, _Sequence, _Category>&,
 		  const _Safe_iterator<_Iterator, _Sequence, _Category>&,
-		  typename _Distance_traits<_Iterator>::__type&);
+		  typename _Distance_traits<_Iterator>::__type&,
+		  bool __skip_if_constexpr);
 
 #if __cplusplus >= 201103L
   template<typename _Iterator,typename _Sequence>
     bool
     __valid_range(const _Safe_local_iterator<_Iterator, _Sequence>&,
 		  const _Safe_local_iterator<_Iterator, _Sequence>&,
-		  typename _Distance_traits<_Iterator>::__type&);
+		  typename _Distance_traits<_Iterator>::__type&,
+		  bool __skip_if_constexpr);
 #endif
 
   template<typename _InputIterator>
     _GLIBCXX14_CONSTEXPR
     inline bool
-    __valid_range(_InputIterator __first, _InputIterator __last)
+    __valid_range(_InputIterator __first, _InputIterator __last,
+		  bool __skip_if_constexpr __attribute__ ((__unused__)))
     {
+#ifdef __cpp_lib_is_constant_evaluated
+      if (std::is_constant_evaluated() && __skip_if_constexpr)
+	return true;
+#endif
       typedef typename std::__is_integer<_InputIterator>::__type _Integral;
       return __valid_range_aux(__first, __last, _Integral());
     }
@@ -269,13 +281,15 @@ namespace __gnu_debug
   template<typename _Iterator, typename _Sequence, typename _Category>
     bool
     __valid_range(const _Safe_iterator<_Iterator, _Sequence, _Category>&,
-		  const _Safe_iterator<_Iterator, _Sequence, _Category>&);
+		  const _Safe_iterator<_Iterator, _Sequence, _Category>&,
+		  bool __skip_if_constexpr);
 
 #if __cplusplus >= 201103L
   template<typename _Iterator, typename _Sequence>
     bool
     __valid_range(const _Safe_local_iterator<_Iterator, _Sequence>&,
-		  const _Safe_local_iterator<_Iterator, _Sequence>&);
+		  const _Safe_local_iterator<_Iterator, _Sequence>&,
+		  bool __skip_if_constexpr);
 #endif
 
   // Fallback method, always ok.
diff --git a/libstdc++-v3/include/debug/macros.h b/libstdc++-v3/include/debug/macros.h
index 9e1288cf4d9..18abd22b717 100644
--- a/libstdc++-v3/include/debug/macros.h
+++ b/libstdc++-v3/include/debug/macros.h
@@ -52,13 +52,17 @@
 #define _GLIBCXX_DEBUG_VERIFY_AT(_Cond,_ErrMsg,_File,_Line)		\
   _GLIBCXX_DEBUG_VERIFY_AT_F(_Cond,_ErrMsg,_File,_Line,__PRETTY_FUNCTION__)
 
-#define _GLIBCXX_DEBUG_VERIFY(_Cond,_ErrMsg)				\
+#ifdef _GLIBCXX_DEBUG
+# define _GLIBCXX_DEBUG_VERIFY(_Cond,_ErrMsg)				\
   _GLIBCXX_DEBUG_VERIFY_AT_F(_Cond, _ErrMsg, __FILE__, __LINE__,	\
 			     __PRETTY_FUNCTION__)
+#else /* _GLIBCXX_ASSERTIONS */
+# define _GLIBCXX_DEBUG_VERIFY(_Cond,_ErrMsg) __glibcxx_assert(_Cond)
+#endif
 
 // Verify that [_First, _Last) forms a valid iterator range.
-#define __glibcxx_check_valid_range(_First,_Last)			\
-_GLIBCXX_DEBUG_VERIFY(__gnu_debug::__valid_range(_First, _Last),	\
+#define __glibcxx_check_valid_range(_First,_Last,_SkipIfConstexpr)	\
+_GLIBCXX_DEBUG_VERIFY(__gnu_debug::__valid_range(_First, _Last,_SkipIfConstexpr), \
 		      _M_message(__gnu_debug::__msg_valid_range)	\
 		      ._M_iterator(_First, #_First)			\
 		      ._M_iterator(_Last, #_Last))
@@ -100,12 +104,12 @@ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__can_advance(_First, _Size),	\
 		      ._M_iterator(_First, #_First)			\
 		      ._M_integer(_Way * _Dist.first, #_Dist))
 
-#define __glibcxx_check_can_increment_range(_First1,_Last1,_First2)	\
+#define __glibcxx_check_can_increment_range(_First1,_Last1,_First2,_SkipIfConstexpr) \
   do									\
   {									\
     typename __gnu_debug::_Distance_traits<__decltype(_First1)>::__type __dist;\
     _GLIBCXX_DEBUG_VERIFY_AT_F(						\
-			__gnu_debug::__valid_range(_First1, _Last1, __dist),\
+			__gnu_debug::__valid_range(_First1, _Last1, __dist, _SkipIfConstexpr), \
 			_M_message(__gnu_debug::__msg_valid_range)	\
 			._M_iterator(_First1, #_First1)			\
 			._M_iterator(_Last1, #_Last1),			\
@@ -238,7 +242,7 @@ _GLIBCXX_DEBUG_VERIFY(_Position._M_attached_to(this),			\
  *  valid iterator range within this sequence.
 */
 #define __glibcxx_check_erase_range(_First,_Last)			\
-__glibcxx_check_valid_range(_First,_Last);				\
+__glibcxx_check_valid_range(_First,_Last,false);			\
 _GLIBCXX_DEBUG_VERIFY(_First._M_attached_to(this),			\
 		      _M_message(__gnu_debug::__msg_erase_different)    \
                       ._M_sequence(*this, "this")			\
@@ -337,7 +341,7 @@ _GLIBCXX_DEBUG_VERIFY(! this->empty(),					\
 
 // Verify that the iterator range [_First, _Last) is sorted
 #define __glibcxx_check_sorted(_First,_Last)				\
-__glibcxx_check_valid_range(_First,_Last);				\
+__glibcxx_check_valid_range(_First,_Last,false);			\
 __glibcxx_check_irreflexive(_First,_Last);				\
  _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__check_sorted(			\
 			__gnu_debug::__base(_First),			\
@@ -349,7 +353,7 @@ __glibcxx_check_irreflexive(_First,_Last);				\
 /** Verify that the iterator range [_First, _Last) is sorted by the
     predicate _Pred. */
 #define __glibcxx_check_sorted_pred(_First,_Last,_Pred)			\
-__glibcxx_check_valid_range(_First,_Last);				\
+__glibcxx_check_valid_range(_First,_Last,false);			\
 __glibcxx_check_irreflexive_pred(_First,_Last,_Pred);			\
 _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__check_sorted(			\
 			__gnu_debug::__base(_First),			\
@@ -361,7 +365,7 @@ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__check_sorted(			\
 
 // Special variant for std::merge, std::includes, std::set_*
 #define __glibcxx_check_sorted_set(_First1,_Last1,_First2)		\
-__glibcxx_check_valid_range(_First1,_Last1);				\
+__glibcxx_check_valid_range(_First1,_Last1,false);			\
 _GLIBCXX_DEBUG_VERIFY(                                                  \
   __gnu_debug::__check_sorted_set(__gnu_debug::__base(_First1),		\
 				  __gnu_debug::__base(_Last1), _First2),\
@@ -371,7 +375,7 @@ _GLIBCXX_DEBUG_VERIFY(                                                  \
 
 // Likewise with a _Pred.
 #define __glibcxx_check_sorted_set_pred(_First1,_Last1,_First2,_Pred)	\
-__glibcxx_check_valid_range(_First1,_Last1);        			\
+__glibcxx_check_valid_range(_First1,_Last1,false);			\
 _GLIBCXX_DEBUG_VERIFY(							\
   __gnu_debug::__check_sorted_set(__gnu_debug::__base(_First1),		\
 				  __gnu_debug::__base(_Last1),		\
@@ -384,7 +388,7 @@ _GLIBCXX_DEBUG_VERIFY(							\
 /** Verify that the iterator range [_First, _Last) is partitioned
     w.r.t. the value _Value. */
 #define __glibcxx_check_partitioned_lower(_First,_Last,_Value)		\
-__glibcxx_check_valid_range(_First,_Last);				\
+__glibcxx_check_valid_range(_First,_Last,false);			\
 _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__check_partitioned_lower(		\
 			__gnu_debug::__base(_First),			\
 			__gnu_debug::__base(_Last), _Value),		\
@@ -394,7 +398,7 @@ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__check_partitioned_lower(		\
 		      ._M_string(#_Value))
 
 #define __glibcxx_check_partitioned_upper(_First,_Last,_Value)		\
-__glibcxx_check_valid_range(_First,_Last);				\
+__glibcxx_check_valid_range(_First,_Last,false);			\
 _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__check_partitioned_upper(		\
 			__gnu_debug::__base(_First),			\
 			__gnu_debug::__base(_Last), _Value),		\
@@ -406,7 +410,7 @@ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__check_partitioned_upper(		\
 /** Verify that the iterator range [_First, _Last) is partitioned
     w.r.t. the value _Value and predicate _Pred. */
 #define __glibcxx_check_partitioned_lower_pred(_First,_Last,_Value,_Pred) \
-__glibcxx_check_valid_range(_First,_Last);				\
+__glibcxx_check_valid_range(_First,_Last,false);			\
 _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__check_partitioned_lower(		\
 			__gnu_debug::__base(_First),			\
 			__gnu_debug::__base(_Last), _Value, _Pred),	\
@@ -419,7 +423,7 @@ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__check_partitioned_lower(		\
 /** Verify that the iterator range [_First, _Last) is partitioned
     w.r.t. the value _Value and predicate _Pred. */
 #define __glibcxx_check_partitioned_upper_pred(_First,_Last,_Value,_Pred) \
-__glibcxx_check_valid_range(_First,_Last);				\
+__glibcxx_check_valid_range(_First,_Last,false);			\
 _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__check_partitioned_upper(		\
 			__gnu_debug::__base(_First),			\
 			__gnu_debug::__base(_Last), _Value, _Pred),	\
diff --git a/libstdc++-v3/testsuite/24_iterators/istream_iterator/1.cc b/libstdc++-v3/testsuite/24_iterators/istream_iterator/1.cc
index 93085aa1471..ecf87a58797 100644
--- a/libstdc++-v3/testsuite/24_iterators/istream_iterator/1.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istream_iterator/1.cc
@@ -48,7 +48,7 @@ void test01()
   ss.str("-1 -2 -3");
   VERIFY( iter == end );
 
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
   // This is undefined, so aborts under debug mode.
   // Without debug mode, it should not extract anything from the stream,
   // and the iterator should remain at end-of-stream.
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..8840f31a1d9
--- /dev/null
+++ b/libstdc++-v3/testsuite/25_algorithms/copy/constexpr_neg.cc
@@ -0,0 +1,52 @@
+// Copyright (C) 2021 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 -D_GLIBCXX_ASSERTIONS" }
+// { 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 "non-constant condition" }
+
+constexpr bool
+test2()
+{
+  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(), ca0.begin() + 8, ma0.begin() + 10);
+
+  return out6 == ma0.begin() + 18;
+}
+
+static_assert(test2()); // { dg-error "is outside the bounds" }
+
+// { dg-prune-output "in 'constexpr' expansion" }
+// { dg-prune-output "builtin_unreachable" }
+// { dg-prune-output "non-constant condition" }
diff --git a/libstdc++-v3/testsuite/25_algorithms/heap/1.cc b/libstdc++-v3/testsuite/25_algorithms/heap/1.cc
index 69793de429f..9be4337729e 100644
--- a/libstdc++-v3/testsuite/25_algorithms/heap/1.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/heap/1.cc
@@ -83,7 +83,7 @@ test02()
 {
   Gt gt;
 
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
   //const int logN = static_cast<int>(std::log(static_cast<double>(N)) + 0.5);
   const int logN = 3;
 #endif
@@ -95,7 +95,7 @@ test02()
   for (int i = 2; i <= N; ++i)
     {
       std::push_heap(s1, s1 + i, gt);
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
       VERIFY(gt.count() <= logN);
 #endif
       gt.reset();
@@ -104,7 +104,7 @@ test02()
   for (int i = N; i >= 2; --i)
     {
       std::pop_heap(s1, s1 + i, gt);
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
       VERIFY(gt.count() <= 2 * logN);
 #endif
       gt.reset();
@@ -118,13 +118,13 @@ test02()
   VERIFY(std::equal(s2, s2 + N, A));
   
   std::make_heap(s2, s2 + N, gt);
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
   VERIFY(gt.count() <= 3 * N);
 #endif
   gt.reset();
 
   std::sort_heap(s2, s2 + N, gt);
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
   VERIFY(gt.count() <= N * logN);
 #endif
   
diff --git a/libstdc++-v3/testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_neg.cc b/libstdc++-v3/testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_neg.cc
index 3536e64c00c..56d80d61ae2 100644
--- a/libstdc++-v3/testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_neg.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_neg.cc
@@ -44,5 +44,5 @@ test()
 
 static_assert(test()); // { dg-error "" }
 
-// { dg-prune-output "failed_assertion" }
+// { dg-prune-output "builtin_unreachable" }
 // { dg-prune-output "in 'constexpr'" }
diff --git a/libstdc++-v3/testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_pred_neg.cc b/libstdc++-v3/testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_pred_neg.cc
index ecda3f5c5da..ab931bbc731 100644
--- a/libstdc++-v3/testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_pred_neg.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_pred_neg.cc
@@ -34,5 +34,5 @@ test()
 
 static_assert(test()); // { dg-error "" }
 
-// { dg-prune-output "failed_assertion" }
+// { dg-prune-output "builtin_unreachable" }
 // { dg-prune-output "in 'constexpr'" }
diff --git a/libstdc++-v3/testsuite/25_algorithms/lower_bound/debug/constexpr_valid_range_neg.cc b/libstdc++-v3/testsuite/25_algorithms/lower_bound/debug/constexpr_valid_range_neg.cc
index e7195438552..fee67e52172 100644
--- a/libstdc++-v3/testsuite/25_algorithms/lower_bound/debug/constexpr_valid_range_neg.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/lower_bound/debug/constexpr_valid_range_neg.cc
@@ -47,6 +47,6 @@ test2()
 
 static_assert(test2()); // { dg-error "" }
 
-// { dg-prune-output "failed_assertion" }
+// { dg-prune-output "builtin_unreachable" }
 // { dg-prune-output "in 'constexpr'" }
 
diff --git a/libstdc++-v3/testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_neg.cc b/libstdc++-v3/testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_neg.cc
index 04d6f73a9ac..7937ac8d308 100644
--- a/libstdc++-v3/testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_neg.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_neg.cc
@@ -44,5 +44,5 @@ test()
 
 static_assert(test()); // { dg-error "" }
 
-// { dg-prune-output "failed_assertion" }
+// { dg-prune-output "builtin_unreachable" }
 // { dg-prune-output "in 'constexpr'" }
diff --git a/libstdc++-v3/testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_pred_neg.cc b/libstdc++-v3/testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_pred_neg.cc
index 7d2ef738c83..7d33eb535fa 100644
--- a/libstdc++-v3/testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_pred_neg.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_pred_neg.cc
@@ -34,5 +34,5 @@ test()
 
 static_assert(test()); // { dg-error "" }
 
-// { dg-prune-output "failed_assertion" }
+// { dg-prune-output "builtin_unreachable" }
 // { dg-prune-output "in 'constexpr'" }
diff --git a/libstdc++-v3/testsuite/25_algorithms/upper_bound/debug/constexpr_valid_range_neg.cc b/libstdc++-v3/testsuite/25_algorithms/upper_bound/debug/constexpr_valid_range_neg.cc
index 33a09b73bca..9c2d0657208 100644
--- a/libstdc++-v3/testsuite/25_algorithms/upper_bound/debug/constexpr_valid_range_neg.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/upper_bound/debug/constexpr_valid_range_neg.cc
@@ -47,5 +47,5 @@ test2()
 
 static_assert(test2()); // { dg-error "" }
 
-// { dg-prune-output "failed_assertion" }
+// { dg-prune-output "builtin_unreachable" }
 // { dg-prune-output "in 'constexpr'" }

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light
  2021-08-06 14:52     ` François Dumont
@ 2021-08-08 19:34       ` François Dumont
  2021-08-23  5:01         ` François Dumont
  0 siblings, 1 reply; 7+ messages in thread
From: François Dumont @ 2021-08-08 19:34 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches

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

After further testing here a fixed version which imply less changes.

Moreover I already commit the fixes unrelated with this patch.

     libstdc++: [_GLIBCXX_ASSERTIONS] Activate basic debug checks

     libstdc++-v3/ChangeLog:

             * include/bits/stl_algobase.h (equal): Use runtime-only 
_GLIBCXX_DEBUG check.
             * include/bits/stl_iterator.h [_GLIBCXX_ASSERTIONS]: 
Include <debug/stl_iterator.h>.
             * include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define debug 
macros non-empty. Most of
             the time do a simple valid_range check.
             * include/debug/helper_functions.h: Cleanup comment about 
removed _Iter_base.
             (__gnu_debug::__valid_range): Add __skip_if_constexpr 
parameter and skip check when true
             and in a constexpr context.
             * include/debug/macros.h (_GLIBCXX_DEBUG_VERIFY): Define as 
__glibcxx_assert when only
             _GLIBCXX_ASSERTIONS is defined.
             (__glibcxx_check_valid_range): Add _SkipIfConstexpr parameter.
             (__glibcxx_check_can_increment_range): Likewise.
             * include/debug/safe_iterator.h (__valid_range): Adapt.
             * include/debug/safe_local_iterator.h (__valid_range): Adapt.
             * testsuite/24_iterators/istream_iterator/1.cc (test01): 
Skip iterator increment when
             _GLIBCXX_ASSERTIONS is defined.
             * testsuite/25_algorithms/copy/constexpr_neg.cc: New test.
             * testsuite/25_algorithms/heap/1.cc: Skip operation 
complexity checks when _GLIBCXX_ASSERTIONS
             is defined.

Ok to commit ?

François


On 06/08/21 4:52 pm, François Dumont wrote:
> On 07/06/21 6:25 am, François Dumont wrote:
>> On 03/06/21 2:31 pm, Jonathan Wakely wrote:
>>>
>>>> +  }
>>>> +
>>>>   /* Checks that [first, last) is a valid range, and then returns
>>>>    * __first. This routine is useful when we can't use a separate
>>>>    * assertion statement because, e.g., we are in a constructor.
>>>> @@ -260,8 +279,9 @@ namespace __gnu_debug
>>>>     inline bool
>>>>     __check_sorted(const _InputIterator& __first, const 
>>>> _InputIterator& __last)
>>>>     {
>>>> -      return __check_sorted_aux(__first, __last,
>>>> -                std::__iterator_category(__first));
>>>> +      return __skip_debug_runtime_check()
>>>> +    || __check_sorted_aux(__first, __last,
>>>> +                  std::__iterator_category(__first));
>>>
>>> Currently this function is never called at all ifndef _GLIBCXX_DEBUG.
>>> With this change, it's going to be present for _GLIBCXX_ASSERTIONS,
>>> and if it isn't inlined it's going to explode the code size.
>>>
>>> Some linux distros are already building the entire distro with
>>> _GLIBCXX_ASSERTIONS so I think we need to be quite careful about this
>>> kind of large change affecting every algo.
>>>
>>> So maybe we shouldn't enable these checks via _GLIBCXX_ASSERTIONS, but
>>> a new macro.
>>>
>> _GLIBCXX_DEBUG is already rarely used, so will be yet another mode.
>>
>> So let's forget about all this, thanks.
>>
> I eventually wonder if your feedback was limited to the use of 
> __check_sorted and some other codes perhaps.
>
> So here is another proposal which activate a small subset of the 
> _GLIBCXX_DEBUG checks in _GLIBCXX_ASSERTIONS but with far less code.
>
> First, the _Error_formatter is not used, the injected checks are 
> simply using __glibcxx_assert.
>
> Second I reduced the number of accitaved checks, mostly the 
> __valid_range.
>
> I also enhance the valid_range check for constexpr because sometimes 
> the normal implementation is good enough to let the compiler diagnose 
> a potential issue in this context. This is for example the case of the 
> std::equal implementation whereas the std::copy implementation is too 
> defensive.
>
>     libstdc++: [_GLIBCXX_ASSERTIONS] Activate basic debug checks
>
>     libstdc++-v3/ChangeLog:
>
>             * include/bits/stl_algobase.h (equal): Use runtime-only 
> _GLIBCXX_DEBUG check.
>             * include/bits/stl_iterator.h [_GLIBCXX_ASSERTIONS]: 
> Include <debug/stl_iterator.h>.
>             * include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define 
> debug macros non-empty. Most of
>             the time do a simple valid_range check.
>             * include/debug/helper_functions.h: Cleanup comment about 
> removed _Iter_base.
>             (__valid_range): Add __skip_if_constexpr parameter and 
> skip check when in a constexpr
>             context.
>             * include/debug/macros.h (_GLIBCXX_DEBUG_VERIFY): Define 
> as __glibcxx_assert when only
>             _GLIBCXX_ASSERTIONS is defined.
>             (__glibcxx_check_valid_range): Add _SkipIfConstexpr 
> parameter.
>             (__glibcxx_check_can_increment_range): Likewise.
>             * testsuite/24_iterators/istream_iterator/1.cc (test01): 
> Skip iterator increment when
>             _GLIBCXX_ASSERTIONS is defined.
>             * testsuite/25_algorithms/copy/constexpr_neg.cc: New test.
>             * testsuite/25_algorithms/heap/1.cc: Skip operation 
> complexity checks when _GLIBCXX_ASSERTIONS
>             is defined.
>             * 
> testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_neg.cc: 
> Fix dg-prune-output reason.
>             * 
> testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_pred_neg.cc: 
> Likewise.
>             * 
> testsuite/25_algorithms/lower_bound/debug/constexpr_valid_range_neg.cc: 
> Likewise.
>             * 
> testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_neg.cc: 
> Likewise.
>             * 
> testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_pred_neg.cc: 
> Likewise.
>             * 
> testsuite/25_algorithms/upper_bound/debug/constexpr_valid_range_neg.cc: 
> Likewise.
>
> The last fixes below are due to the recent changes to the 
> __glibcxx_assert macro but it is close to the code I am changing so I 
> prefer to fix those here.
>
> Tested under Linux x86_64 w/o _GLIBCXX_ASSERTIONS.
>
> Ok to commit ?
>
> François
>


[-- Attachment #2: assertion_mode.patch --]
[-- Type: text/x-patch, Size: 15915 bytes --]

diff --git a/libstdc++-v3/include/bits/stl_algobase.h b/libstdc++-v3/include/bits/stl_algobase.h
index d0c49628d7f..d45eec4339b 100644
--- a/libstdc++-v3/include/bits/stl_algobase.h
+++ b/libstdc++-v3/include/bits/stl_algobase.h
@@ -1551,7 +1551,7 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO
       __glibcxx_function_requires(_EqualOpConcept<
 	    typename iterator_traits<_II1>::value_type,
 	    typename iterator_traits<_II2>::value_type>)
-      __glibcxx_requires_can_increment_range(__first1, __last1, __first2);
+      __glibcxx_requires_can_increment_range_runtime(__first1, __last1, __first2);
 
       return std::__equal_aux(__first1, __last1, __first2);
     }
diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
index c5b02408c1c..a8986d11dfe 100644
--- a/libstdc++-v3/include/bits/stl_iterator.h
+++ b/libstdc++-v3/include/bits/stl_iterator.h
@@ -2463,7 +2463,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace
 
-#ifdef _GLIBCXX_DEBUG
+#ifdef _GLIBCXX_ASSERTIONS
 # include <debug/stl_iterator.h>
 #endif
 
diff --git a/libstdc++-v3/include/debug/debug.h b/libstdc++-v3/include/debug/debug.h
index 116f2f023e2..4ff027eaa74 100644
--- a/libstdc++-v3/include/debug/debug.h
+++ b/libstdc++-v3/include/debug/debug.h
@@ -61,12 +61,13 @@ namespace __gnu_debug
     struct _Safe_iterator;
 }
 
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
 
 # define __glibcxx_requires_cond(_Cond,_Msg)
 # define __glibcxx_requires_valid_range(_First,_Last)
 # define __glibcxx_requires_can_increment(_First,_Size)
 # define __glibcxx_requires_can_increment_range(_First1,_Last1,_First2)
+# define __glibcxx_requires_can_increment_range_runtime(_First1,_Last1,_First2)
 # define __glibcxx_requires_can_decrement_range(_First1,_Last1,_First2)
 # define __glibcxx_requires_sorted(_First,_Last)
 # define __glibcxx_requires_sorted_pred(_First,_Last,_Pred)
@@ -87,6 +88,7 @@ namespace __gnu_debug
 
 #else
 
+# ifdef _GLIBCXX_DEBUG
 #  include <debug/macros.h>
 
 #  define __glibcxx_requires_cond(_Cond,_Msg) _GLIBCXX_DEBUG_VERIFY(_Cond,_Msg)
@@ -95,7 +97,9 @@ namespace __gnu_debug
 #  define __glibcxx_requires_can_increment(_First,_Size)	\
   __glibcxx_check_can_increment(_First,_Size)
 #  define __glibcxx_requires_can_increment_range(_First1,_Last1,_First2) \
-  __glibcxx_check_can_increment_range(_First1,_Last1,_First2)
+  __glibcxx_check_can_increment_range(_First1,_Last1,_First2,false)
+#  define __glibcxx_requires_can_increment_range_runtime(_First1,_Last1,_First2) \
+  __glibcxx_check_can_increment_range(_First1,_Last1,_First2,true)
 #  define __glibcxx_requires_can_decrement_range(_First1,_Last1,_First2)	\
   __glibcxx_check_can_decrement_range(_First1,_Last1,_First2)
 #  define __glibcxx_requires_sorted(_First,_Last)	\
@@ -131,7 +135,50 @@ namespace __gnu_debug
   __glibcxx_check_irreflexive_pred2(_First,_Last,_Pred)
 
 # include <debug/functions.h>
+# else
+#  define __glibcxx_requires_cond(_Cond,_Msg) __glibcxx_assert(_Cond)
+#  define __glibcxx_requires_valid_range(_First,_Last)	\
+  __glibcxx_assert(__gnu_debug::__valid_range(_First,_Last))
+#  define __glibcxx_requires_can_increment(_First,_Size)	\
+  __glibcxx_assert(__gnu_debug::__can_advance(_First,_Size))
+#  define __glibcxx_requires_can_increment_range(_First1,_Last1,_First2) \
+  __glibcxx_requires_valid_range(_First1,_Last1)
+#  define __glibcxx_requires_can_increment_range_runtime(_First1,_Last1,_First2) \
+  __glibcxx_assert(__gnu_debug::__valid_range(_First1,_Last1,true))
+#  define __glibcxx_requires_can_decrement_range(_First1,_Last1,_First2)	\
+  __glibcxx_requires_valid_range(_First1,_Last1)
+#  define __glibcxx_requires_sorted(_First,_Last)			\
+  __glibcxx_requires_valid_range(_First,_Last);				\
+  __glibcxx_requires_irreflexive(_First,_Last)
+#  define __glibcxx_requires_sorted_pred(_First,_Last,_Pred)		\
+  __glibcxx_requires_valid_range(_First,_Last);				\
+  __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred)
+#  define __glibcxx_requires_sorted_set(_First1,_Last1,_First2)		\
+  __glibcxx_requires_valid_range(_First1,_Last1)
+#  define __glibcxx_requires_sorted_set_pred(_First1,_Last1,_First2,_Pred) \
+  __glibcxx_requires_valid_range(_First1,_Last1)
+#  define __glibcxx_requires_partitioned_lower(_First,_Last,_Value)	\
+  __glibcxx_requires_valid_range(_First,_Last)
+#  define __glibcxx_requires_partitioned_upper(_First,_Last,_Value)	\
+  __glibcxx_requires_valid_range(_First,_Last)
+#  define __glibcxx_requires_partitioned_lower_pred(_First,_Last,_Value,_Pred) \
+  __glibcxx_requires_valid_range(_First,_Last)
+#  define __glibcxx_requires_partitioned_upper_pred(_First,_Last,_Value,_Pred) \
+  __glibcxx_requires_valid_range(_First,_Last)
+#  define __glibcxx_requires_heap(_First,_Last)
+#  define __glibcxx_requires_heap_pred(_First,_Last,_Pred)
+#  define __glibcxx_requires_string(_String)
+#  define __glibcxx_requires_string_len(_String,_Len)
+#  define __glibcxx_requires_irreflexive(_First,_Last)	\
+  __glibcxx_assert(_First == _Last || !(*_First < *_First))
+#  define __glibcxx_requires_irreflexive2(_First,_Last)
+#  define __glibcxx_requires_irreflexive_pred(_First,_Last,_Pred)	\
+  __glibcxx_assert(_First == _Last || !_Pred(*_First < *_First))
+#  define __glibcxx_requires_irreflexive_pred2(_First,_Last,_Pred)
+
+#  include <debug/helper_functions.h>
+# endif /* _GLIBCXX_DEBUG */
 
-#endif
+#endif /* _GLIBCXX_ASSERTIONS */
 
 #endif // _GLIBCXX_DEBUG_MACRO_SWITCH_H
diff --git a/libstdc++-v3/include/debug/helper_functions.h b/libstdc++-v3/include/debug/helper_functions.h
index c0144ced979..e4e582aefb9 100644
--- a/libstdc++-v3/include/debug/helper_functions.h
+++ b/libstdc++-v3/include/debug/helper_functions.h
@@ -30,8 +30,8 @@
 #define _GLIBCXX_DEBUG_HELPER_FUNCTIONS_H 1
 
 #include <bits/move.h>				// for __addressof
-#include <bits/stl_iterator_base_types.h>	// for iterator_traits,
-						// categories and _Iter_base
+#include <bits/stl_iterator_base_types.h>	// for iterator_traits and
+						// categories
 #include <bits/cpp_type_traits.h>		// for __is_integer
 
 #include <bits/stl_pair.h>			// for pair
@@ -237,8 +237,13 @@ namespace __gnu_debug
     _GLIBCXX20_CONSTEXPR
     inline bool
     __valid_range(_InputIterator __first, _InputIterator __last,
-		  typename _Distance_traits<_InputIterator>::__type& __dist)
+		  typename _Distance_traits<_InputIterator>::__type& __dist,
+		  bool __skip_if_constexpr __attribute__ ((__unused__)) = false)
     {
+#ifdef __cpp_lib_is_constant_evaluated
+      if (std::is_constant_evaluated() && __skip_if_constexpr)
+	return true;
+#endif
       typedef typename std::__is_integer<_InputIterator>::__type _Integral;
       return __valid_range_aux(__first, __last, __dist, _Integral());
     }
@@ -247,21 +252,28 @@ namespace __gnu_debug
     bool
     __valid_range(const _Safe_iterator<_Iterator, _Sequence, _Category>&,
 		  const _Safe_iterator<_Iterator, _Sequence, _Category>&,
-		  typename _Distance_traits<_Iterator>::__type&);
+		  typename _Distance_traits<_Iterator>::__type&,
+		  bool __skip_if_constexpr = false);
 
 #if __cplusplus >= 201103L
   template<typename _Iterator,typename _Sequence>
     bool
     __valid_range(const _Safe_local_iterator<_Iterator, _Sequence>&,
 		  const _Safe_local_iterator<_Iterator, _Sequence>&,
-		  typename _Distance_traits<_Iterator>::__type&);
+		  typename _Distance_traits<_Iterator>::__type&,
+		  bool __skip_if_constexpr = false);
 #endif
 
   template<typename _InputIterator>
     _GLIBCXX14_CONSTEXPR
     inline bool
-    __valid_range(_InputIterator __first, _InputIterator __last)
+    __valid_range(_InputIterator __first, _InputIterator __last,
+		  bool __skip_if_constexpr __attribute__ ((__unused__)) = false)
     {
+#ifdef __cpp_lib_is_constant_evaluated
+      if (std::is_constant_evaluated() && __skip_if_constexpr)
+	return true;
+#endif
       typedef typename std::__is_integer<_InputIterator>::__type _Integral;
       return __valid_range_aux(__first, __last, _Integral());
     }
@@ -269,13 +281,15 @@ namespace __gnu_debug
   template<typename _Iterator, typename _Sequence, typename _Category>
     bool
     __valid_range(const _Safe_iterator<_Iterator, _Sequence, _Category>&,
-		  const _Safe_iterator<_Iterator, _Sequence, _Category>&);
+		  const _Safe_iterator<_Iterator, _Sequence, _Category>&,
+		  bool __skip_if_constexpr = false);
 
 #if __cplusplus >= 201103L
   template<typename _Iterator, typename _Sequence>
     bool
     __valid_range(const _Safe_local_iterator<_Iterator, _Sequence>&,
-		  const _Safe_local_iterator<_Iterator, _Sequence>&);
+		  const _Safe_local_iterator<_Iterator, _Sequence>&,
+		  bool __skip_if_constexpr = false);
 #endif
 
   // Fallback method, always ok.
diff --git a/libstdc++-v3/include/debug/macros.h b/libstdc++-v3/include/debug/macros.h
index 9e1288cf4d9..5332ad09d22 100644
--- a/libstdc++-v3/include/debug/macros.h
+++ b/libstdc++-v3/include/debug/macros.h
@@ -100,12 +100,12 @@ _GLIBCXX_DEBUG_VERIFY(__gnu_debug::__can_advance(_First, _Size),	\
 		      ._M_iterator(_First, #_First)			\
 		      ._M_integer(_Way * _Dist.first, #_Dist))
 
-#define __glibcxx_check_can_increment_range(_First1,_Last1,_First2)	\
+#define __glibcxx_check_can_increment_range(_First1,_Last1,_First2,_SkipIfConstexpr) \
   do									\
   {									\
     typename __gnu_debug::_Distance_traits<__decltype(_First1)>::__type __dist;\
     _GLIBCXX_DEBUG_VERIFY_AT_F(						\
-			__gnu_debug::__valid_range(_First1, _Last1, __dist),\
+			__gnu_debug::__valid_range(_First1, _Last1, __dist, _SkipIfConstexpr), \
 			_M_message(__gnu_debug::__msg_valid_range)	\
 			._M_iterator(_First1, #_First1)			\
 			._M_iterator(_Last1, #_Last1),			\
diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h
index 5584d06de5a..003557f2e59 100644
--- a/libstdc++-v3/include/debug/safe_iterator.h
+++ b/libstdc++-v3/include/debug/safe_iterator.h
@@ -965,7 +965,8 @@ namespace __gnu_debug
 				       _Category>& __first,
 		  const _Safe_iterator<_Iterator, _Sequence,
 				       _Category>& __last,
-		  typename _Distance_traits<_Iterator>::__type& __dist)
+		  typename _Distance_traits<_Iterator>::__type& __dist,
+		  bool /* __skip_if_constexpr */)
     { return __first._M_valid_range(__last, __dist); }
 
   template<typename _Iterator, typename _Sequence, typename _Category>
@@ -973,7 +974,8 @@ namespace __gnu_debug
     __valid_range(const _Safe_iterator<_Iterator, _Sequence,
 				       _Category>& __first,
 		  const _Safe_iterator<_Iterator, _Sequence,
-				       _Category>& __last)
+				       _Category>& __last,
+		  bool /* __skip_if_constexpr */)
     {
       typename _Distance_traits<_Iterator>::__type __dist;
       return __first._M_valid_range(__last, __dist);
diff --git a/libstdc++-v3/include/debug/safe_local_iterator.h b/libstdc++-v3/include/debug/safe_local_iterator.h
index 31c48e65a24..732965860c0 100644
--- a/libstdc++-v3/include/debug/safe_local_iterator.h
+++ b/libstdc++-v3/include/debug/safe_local_iterator.h
@@ -408,13 +408,15 @@ namespace __gnu_debug
     inline bool
     __valid_range(const _Safe_local_iterator<_Iterator, _Sequence>& __first,
 		  const _Safe_local_iterator<_Iterator, _Sequence>& __last,
-		  typename _Distance_traits<_Iterator>::__type& __dist_info)
+		  typename _Distance_traits<_Iterator>::__type& __dist_info,
+		  bool /* __skip_if_constexpr */)
     { return __first._M_valid_range(__last, __dist_info); }
 
   template<typename _Iterator, typename _Sequence>
     inline bool
     __valid_range(const _Safe_local_iterator<_Iterator, _Sequence>& __first,
-		  const _Safe_local_iterator<_Iterator, _Sequence>& __last)
+		  const _Safe_local_iterator<_Iterator, _Sequence>& __last,
+		  bool /* __skip_if_constexpr */)
     {
       typename _Distance_traits<_Iterator>::__type __dist_info;
       return __first._M_valid_range(__last, __dist_info);
diff --git a/libstdc++-v3/testsuite/24_iterators/istream_iterator/1.cc b/libstdc++-v3/testsuite/24_iterators/istream_iterator/1.cc
index 93085aa1471..ecf87a58797 100644
--- a/libstdc++-v3/testsuite/24_iterators/istream_iterator/1.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istream_iterator/1.cc
@@ -48,7 +48,7 @@ void test01()
   ss.str("-1 -2 -3");
   VERIFY( iter == end );
 
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
   // This is undefined, so aborts under debug mode.
   // Without debug mode, it should not extract anything from the stream,
   // and the iterator should remain at end-of-stream.
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..8840f31a1d9
--- /dev/null
+++ b/libstdc++-v3/testsuite/25_algorithms/copy/constexpr_neg.cc
@@ -0,0 +1,52 @@
+// Copyright (C) 2021 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 -D_GLIBCXX_ASSERTIONS" }
+// { 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 "non-constant condition" }
+
+constexpr bool
+test2()
+{
+  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(), ca0.begin() + 8, ma0.begin() + 10);
+
+  return out6 == ma0.begin() + 18;
+}
+
+static_assert(test2()); // { dg-error "is outside the bounds" }
+
+// { dg-prune-output "in 'constexpr' expansion" }
+// { dg-prune-output "builtin_unreachable" }
+// { dg-prune-output "non-constant condition" }
diff --git a/libstdc++-v3/testsuite/25_algorithms/heap/1.cc b/libstdc++-v3/testsuite/25_algorithms/heap/1.cc
index 69793de429f..9be4337729e 100644
--- a/libstdc++-v3/testsuite/25_algorithms/heap/1.cc
+++ b/libstdc++-v3/testsuite/25_algorithms/heap/1.cc
@@ -83,7 +83,7 @@ test02()
 {
   Gt gt;
 
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
   //const int logN = static_cast<int>(std::log(static_cast<double>(N)) + 0.5);
   const int logN = 3;
 #endif
@@ -95,7 +95,7 @@ test02()
   for (int i = 2; i <= N; ++i)
     {
       std::push_heap(s1, s1 + i, gt);
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
       VERIFY(gt.count() <= logN);
 #endif
       gt.reset();
@@ -104,7 +104,7 @@ test02()
   for (int i = N; i >= 2; --i)
     {
       std::pop_heap(s1, s1 + i, gt);
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
       VERIFY(gt.count() <= 2 * logN);
 #endif
       gt.reset();
@@ -118,13 +118,13 @@ test02()
   VERIFY(std::equal(s2, s2 + N, A));
   
   std::make_heap(s2, s2 + N, gt);
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
   VERIFY(gt.count() <= 3 * N);
 #endif
   gt.reset();
 
   std::sort_heap(s2, s2 + N, gt);
-#ifndef _GLIBCXX_DEBUG
+#ifndef _GLIBCXX_ASSERTIONS
   VERIFY(gt.count() <= N * logN);
 #endif
   

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light
  2021-08-08 19:34       ` François Dumont
@ 2021-08-23  5:01         ` François Dumont
  0 siblings, 0 replies; 7+ messages in thread
From: François Dumont @ 2021-08-23  5:01 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: libstdc++, gcc-patches

Any feedback ?

Thanks

On 08/08/21 9:34 pm, François Dumont wrote:
> After further testing here a fixed version which imply less changes.
>
> Moreover I already commit the fixes unrelated with this patch.
>
>     libstdc++: [_GLIBCXX_ASSERTIONS] Activate basic debug checks
>
>     libstdc++-v3/ChangeLog:
>
>             * include/bits/stl_algobase.h (equal): Use runtime-only 
> _GLIBCXX_DEBUG check.
>             * include/bits/stl_iterator.h [_GLIBCXX_ASSERTIONS]: 
> Include <debug/stl_iterator.h>.
>             * include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define 
> debug macros non-empty. Most of
>             the time do a simple valid_range check.
>             * include/debug/helper_functions.h: Cleanup comment about 
> removed _Iter_base.
>             (__gnu_debug::__valid_range): Add __skip_if_constexpr 
> parameter and skip check when true
>             and in a constexpr context.
>             * include/debug/macros.h (_GLIBCXX_DEBUG_VERIFY): Define 
> as __glibcxx_assert when only
>             _GLIBCXX_ASSERTIONS is defined.
>             (__glibcxx_check_valid_range): Add _SkipIfConstexpr 
> parameter.
>             (__glibcxx_check_can_increment_range): Likewise.
>             * include/debug/safe_iterator.h (__valid_range): Adapt.
>             * include/debug/safe_local_iterator.h (__valid_range): Adapt.
>             * testsuite/24_iterators/istream_iterator/1.cc (test01): 
> Skip iterator increment when
>             _GLIBCXX_ASSERTIONS is defined.
>             * testsuite/25_algorithms/copy/constexpr_neg.cc: New test.
>             * testsuite/25_algorithms/heap/1.cc: Skip operation 
> complexity checks when _GLIBCXX_ASSERTIONS
>             is defined.
>
> Ok to commit ?
>
> François
>
>
> On 06/08/21 4:52 pm, François Dumont wrote:
>> On 07/06/21 6:25 am, François Dumont wrote:
>>> On 03/06/21 2:31 pm, Jonathan Wakely wrote:
>>>>
>>>>> +  }
>>>>> +
>>>>>   /* Checks that [first, last) is a valid range, and then returns
>>>>>    * __first. This routine is useful when we can't use a separate
>>>>>    * assertion statement because, e.g., we are in a constructor.
>>>>> @@ -260,8 +279,9 @@ namespace __gnu_debug
>>>>>     inline bool
>>>>>     __check_sorted(const _InputIterator& __first, const 
>>>>> _InputIterator& __last)
>>>>>     {
>>>>> -      return __check_sorted_aux(__first, __last,
>>>>> -                std::__iterator_category(__first));
>>>>> +      return __skip_debug_runtime_check()
>>>>> +    || __check_sorted_aux(__first, __last,
>>>>> +                  std::__iterator_category(__first));
>>>>
>>>> Currently this function is never called at all ifndef _GLIBCXX_DEBUG.
>>>> With this change, it's going to be present for _GLIBCXX_ASSERTIONS,
>>>> and if it isn't inlined it's going to explode the code size.
>>>>
>>>> Some linux distros are already building the entire distro with
>>>> _GLIBCXX_ASSERTIONS so I think we need to be quite careful about this
>>>> kind of large change affecting every algo.
>>>>
>>>> So maybe we shouldn't enable these checks via _GLIBCXX_ASSERTIONS, but
>>>> a new macro.
>>>>
>>> _GLIBCXX_DEBUG is already rarely used, so will be yet another mode.
>>>
>>> So let's forget about all this, thanks.
>>>
>> I eventually wonder if your feedback was limited to the use of 
>> __check_sorted and some other codes perhaps.
>>
>> So here is another proposal which activate a small subset of the 
>> _GLIBCXX_DEBUG checks in _GLIBCXX_ASSERTIONS but with far less code.
>>
>> First, the _Error_formatter is not used, the injected checks are 
>> simply using __glibcxx_assert.
>>
>> Second I reduced the number of accitaved checks, mostly the 
>> __valid_range.
>>
>> I also enhance the valid_range check for constexpr because sometimes 
>> the normal implementation is good enough to let the compiler diagnose 
>> a potential issue in this context. This is for example the case of 
>> the std::equal implementation whereas the std::copy implementation is 
>> too defensive.
>>
>>     libstdc++: [_GLIBCXX_ASSERTIONS] Activate basic debug checks
>>
>>     libstdc++-v3/ChangeLog:
>>
>>             * include/bits/stl_algobase.h (equal): Use runtime-only 
>> _GLIBCXX_DEBUG check.
>>             * include/bits/stl_iterator.h [_GLIBCXX_ASSERTIONS]: 
>> Include <debug/stl_iterator.h>.
>>             * include/debug/debug.h [_GLIBCXX_ASSERTIONS]: Define 
>> debug macros non-empty. Most of
>>             the time do a simple valid_range check.
>>             * include/debug/helper_functions.h: Cleanup comment about 
>> removed _Iter_base.
>>             (__valid_range): Add __skip_if_constexpr parameter and 
>> skip check when in a constexpr
>>             context.
>>             * include/debug/macros.h (_GLIBCXX_DEBUG_VERIFY): Define 
>> as __glibcxx_assert when only
>>             _GLIBCXX_ASSERTIONS is defined.
>>             (__glibcxx_check_valid_range): Add _SkipIfConstexpr 
>> parameter.
>>             (__glibcxx_check_can_increment_range): Likewise.
>>             * testsuite/24_iterators/istream_iterator/1.cc (test01): 
>> Skip iterator increment when
>>             _GLIBCXX_ASSERTIONS is defined.
>>             * testsuite/25_algorithms/copy/constexpr_neg.cc: New test.
>>             * testsuite/25_algorithms/heap/1.cc: Skip operation 
>> complexity checks when _GLIBCXX_ASSERTIONS
>>             is defined.
>>             * 
>> testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_neg.cc: 
>> Fix dg-prune-output reason.
>>             * 
>> testsuite/25_algorithms/lower_bound/debug/constexpr_partitioned_pred_neg.cc: 
>> Likewise.
>>             * 
>> testsuite/25_algorithms/lower_bound/debug/constexpr_valid_range_neg.cc: 
>> Likewise.
>>             * 
>> testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_neg.cc: 
>> Likewise.
>>             * 
>> testsuite/25_algorithms/upper_bound/debug/constexpr_partitioned_pred_neg.cc: 
>> Likewise.
>>             * 
>> testsuite/25_algorithms/upper_bound/debug/constexpr_valid_range_neg.cc: 
>> Likewise.
>>
>> The last fixes below are due to the recent changes to the 
>> __glibcxx_assert macro but it is close to the code I am changing so I 
>> prefer to fix those here.
>>
>> Tested under Linux x86_64 w/o _GLIBCXX_ASSERTIONS.
>>
>> Ok to commit ?
>>
>> François
>>
>


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-08-23  5:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-27 17:37 [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light François Dumont
2021-05-31 17:17 ` François Dumont
2021-06-03 12:31 ` Jonathan Wakely
2021-06-07  4:25   ` François Dumont
2021-08-06 14:52     ` François Dumont
2021-08-08 19:34       ` François Dumont
2021-08-23  5:01         ` François Dumont

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).