Re: [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light

["François Dumont" <frs.dumont@gmail.com>]

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