Limit Debug mode impact: overload __niter_base

[François Dumont]

On 27/06/2018 22:02, Jonathan Wakely wrote:
> On 27/06/18 21:25 +0200, François Dumont wrote:
>> 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 do for my version of the patch, because with the new __niter_base
> overload the debug layer gets removed. Your patch kept the checking by
> not removing the debug layer, but loses the memcmp optimization.
>
> My suggestion above removes the debug layer so uses the optimization,
> but adds a separate range check, so we still get checking as well.
>
>> 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());
>
> We should assert for that test, it's undefined behaviour.

Oh, ok, then __glibcxx_requires_can_increment_range can be called 
unconditionnaly. It already takes care of doing the right thing.

And some tests might need to be fixed because they contain this 
undefined behavior. I'll do it.

François