[PATCH][Bug libstdc++/70303] Value-initialized debug iterators

[François Dumont]

On 01/02/21 8:09 pm, Jonathan Wakely wrote:
> On 01/02/21 19:30 +0100, François Dumont via Libstdc++ wrote:
>> On 01/02/21 6:43 pm, Jonathan Wakely wrote:
>>> On 31/01/21 16:59 +0100, François Dumont via Libstdc++ wrote:
>>>> After the debug issue has been fixed in PR 98466 the problem was 
>>>> not in the debug iterator implementation itself but in the deque 
>>>> iterator operator- implementation.
>>>>
>>>>     libstdc++: Make deque iterator operator- usable with value-init 
>>>> iterators
>>>>
>>>>     N3644 implies that operator- can be used on value-init 
>>>> iterators. We now return
>>>>     0 if both iterators are value initialized. If only one is value 
>>>> initialized we
>>>>     keep the UB by returning the result of a normal computation 
>>>> which is an unexpected
>>>>     value.
>>>>
>>>>     libstdc++/ChangeLog:
>>>>
>>>>             PR libstdc++/70303
>>>>             * include/bits/stl_deque.h 
>>>> (std::deque<>::operator-(iterator, iterator)):
>>>>             Return 0 if both iterators are value-initialized.
>>>>             * testsuite/23_containers/deque/70303.cc: New test.
>>>>             * testsuite/23_containers/vector/70303.cc: New test.
>>>>
>>>> Tested under Linux x86_64, ok to commit ?
>>>
>>> OK.
>>>
>>> I don't like adding the branch there though. Even with the
>>> __builtin_expect it causes worse code to be generated than the
>>> original.
>>>
>>> This would be branchless, but a bit harder to understand:
>>>
>>>     return difference_type(__x._S_buffer_size())
>>>       * (__x._M_node - __y._M_node - int(__x._M_node == __y._M_node))
>>>       + (__x._M_cur - __x._M_first) + (__y._M_last - __y._M_cur);
>>>
>>> Please commit the fix and we can think about it later.
>>
>> I do not think this work because for value-init iterators we have 
>> __x._M_node == __y._M_node == nullptr so the diff would give 
>> -_S_buffer_size().
>>
>> But I got the idear, I'll prepare the patch.
>
> Yeah, I just came back to the computer to say that it's wrong. But
> maybe this:
>
>     return difference_type(_S_buffer_size())
>       * (__x._M_node - __y._M_node - int(__x._M_node && __y._M_node))
>       + (__x._M_cur - __x._M_first) + (__y._M_last - __y._M_cur);
>
> For value-init'd iterators we'd get _S_buffer_size() * 0 + 0 - 0
>
> We could even just use int(__x._M_node != 0) because if one is null
> and the other isn't, it's UB anyway.
>
>
This is what I've tested with success. As it is a recent kind of 
regression you might want to consider it now.

     libstdc++: Remove execution branch in deque iterator operator-

     libstdc++-v3/ChangeLog:

             * include/bits/stl_deque.h
             (std::operator-(deque::iterator, deque::iterator)): Replace 
if/then with
             a null pointer test.

Ok to commit ?

François

-------------- next part --------------
A non-text attachment was scrubbed...
Name: deque_ite_operator.patch
Type: text/x-patch
Size: 1423 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20210208/5c2d9be7/attachment.bin>