Limit Debug mode impact: overload __niter_base

[François Dumont]

On 27/06/2018 02:13, Jonathan Wakely wrote:
> On 26/06/18 17:03 +0100, Jonathan Wakely wrote:
>> On 18/06/18 23:01 +0200, François Dumont wrote:
>>> Hi
>>>
>>> Â Â Â  I abandon the idea of providing Debug algos, it would be too 
>>> much code to add and maintain. However I haven't quit on reducing 
>>> Debug mode performance impact.
>>>
>>> Â Â Â  So this patch make use of the existing std::__niter_base to get 
>>> rid of the debug layer around __gnu_debug::vector<>::iterator so 
>>> that __builtin_memmove replacement can take place.
>>>
>>> Â Â Â  As _Safe_iterator<> do not have a constructor taking a pointer I 
>>> change algos implementation so that we do not try to instantiate the 
>>> iterator type ourselves but rather rely on its operators + or -.
>>>
>>> Â Â Â  The small drawback is that for std::equal algo where we can't 
>>> use the __glibcxx_can_increment we need to keep the debug layer to 
>>> make sure we don't reach past-the-end iterator. So I had to remove 
>>> usage of __niter_base when in Debug mode, doing so it also disable 
>>> potential usage of __builtin_memcmp when calling std::equal on 
>>> std::__cxx1998::vector<> iterators. A rather rare situation I think.
>
> We don't need to give up all checks in std::equal, we can do this:
>
> @@ -1044,7 +1085,13 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO
> Â Â Â Â Â Â Â Â Â Â  typename iterator_traits<_II1>::value_type,
> Â Â Â Â Â Â Â Â Â Â  typename iterator_traits<_II2>::value_type>)
> Â Â Â Â Â  __glibcxx_requires_valid_range(__first1, __last1);
> -
> +#ifdef _GLIBCXX_DEBUG
> +Â Â Â Â Â  typedef typename iterator_traits<_II1>::iterator_category _Cat1;
> +Â Â Â Â Â  typedef typename iterator_traits<_II2>::iterator_category _Cat2;
> +Â Â Â Â Â  if (!__are_same<_Cat1, input_iterator_tag>::__value
> +Â Â Â Â Â Â Â Â  && __are_same<_Cat2, random_access_iterator_tag>::__value)
> +Â Â Â Â Â Â  __glibcxx_requires_can_increment_range(__first1, __last1, 
> __first2);
> +#endif
> Â Â Â Â Â  return std::__equal_aux(std::__niter_base(__first1),
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  std::__niter_base(__last1),
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  std::__niter_base(__first2));

We don't give up any check. We only give up on using the 
__builtin_memcmp when std::equal is being used while Debug mode is 
active and std::equal is called directly with std::__cxx1998::vector.

Moreover this check is wrong and will introduce regression when running 
testsuite in Debug mode (this is why I know). It will assert in the 
following case:
vector<int> v1 { 0, 0, 0 };
vector<int> v2 { 0, 1 };
equal(v1.begin(), v1.end(), v2.begin());

>
>
>
>>> Â Â Â  Note that I don't know how to test that __builtin_memmove has 
>>> been indeed used. So I've been through some debug sessions to check 
>>> that.
>>
>> The attached patch (not fully tested) seems to be a much simpler way
>> to achieve the same thing. Instead of modifying all the helper
>> structs, just define a new function to re-wrap the result into the
>> desired iterator type.
>>
>>
>>> diff --git a/libstdc++-v3/include/debug/stl_iterator.h 
>>> b/libstdc++-v3/include/debug/stl_iterator.h
>>> index a6a2a76..eca7203 100644
>>> --- a/libstdc++-v3/include/debug/stl_iterator.h
>>> +++ b/libstdc++-v3/include/debug/stl_iterator.h
>>> @@ -120,4 +120,19 @@ namespace __gnu_debug
>>> #endif
>>> }
>>>
>>> +#if __cplusplus >= 201103L
>>> +namespace std
>>> +{
>>> +_GLIBCXX_BEGIN_NAMESPACE_VERSION
>>> +
>>> +template<typename _Iterator, typename _Container, typename _Sequence>
>>> +Â Â Â  _Iterator
>>> +Â Â Â  __niter_base(const __gnu_debug::_Safe_iterator<
>>> +Â Â Â Â Â Â Â Â  __gnu_cxx::__normal_iterator<_Iterator, _Container>,
>>> +Â Â Â Â Â Â Â Â  _Sequence>&);
>>> +
>>> +_GLIBCXX_END_NAMESPACE_VERSION
>>> +}
>>> +#endif
>>
>> Why is this overload only defined for C++11 and later? I defined it
>> unconditionally in the attached patch.
>>
>> What do you think?
>
> Here's a complete patch that passes all tests in normal mode and
> causes no regressions in debug mode (we already have some debug test
> failing).
>
> I wondered whether we need another overload of __wrap_iter for
> handling move_iterator, but I think the first overload works OK.
>
>
I don't think we need it neither. Algos that handle move iterator are 
already doing it.

François