Re: [PATCH] Use _GLIBCXX_ASSERTIONS as _GLIBCXX_DEBUG light

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

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