Re: [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light

[Jonathan Wakely <jwakely@redhat.com>]

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.