On 08/08/22 15:19, Jonathan Wakely wrote:
> On Mon, 8 Aug 2022 at 06:07, François Dumont via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
>> Another version of this patch with just a new test case showing what
>> wrong code was unnoticed previously by the _GLIBCXX_DEBUG mode.
>>
>> On 04/08/22 22:56, François Dumont wrote:
>>> This an old patch I had prepared a long time ago, I don't think I ever
>>> submitted it.
>>>
>>>      libstdc++: [_GLIBCXX_DEBUG] Do not consider detached iterators as
>>> value-initialized
>>>
>>>      An attach iterator has its _M_version set to something != 0. This
>>> value shall be preserved
>>>      when detaching it so that the iterator does not look like a value
>>> initialized one.
>>>
>>>      libstdc++-v3/ChangeLog:
>>>
>>>              * include/debug/formatter.h (__singular_value_init): New
>>> _Iterator_state enum entry.
>>>              (_Parameter<>(const _Safe_iterator<>&, const char*,
>>> _Is_iterator)): Check if iterator
>>>              parameter is value-initialized.
>>>              (_Parameter<>(const _Safe_local_iterator<>&, const char*,
>>> _Is_iterator)): Likewise.
>>>              * include/debug/safe_iterator.h
>>> (_Safe_iterator<>::_M_value_initialized()): New. Adapt
>>>              checks.
>>>              * include/debug/safe_local_iterator.h
>>> (_Safe_local_iterator<>::_M_value_initialized()): New.
>>>              Adapt checks.
>>>              * src/c++11/debug.cc (_Safe_iterator_base::_M_reset): Do
>>> not reset _M_version.
>>>              (print_field(PrintContext&, const _Parameter&, const
>>> char*)): Adapt state_names.
>>>              * testsuite/23_containers/deque/debug/iterator1_neg.cc:
>>> New test.
>>>              * testsuite/23_containers/deque/debug/iterator2_neg.cc:
>>> New test.
>>>              *
>>> testsuite/23_containers/forward_list/debug/iterator1_neg.cc: New test.
>>>              *
>>> testsuite/23_containers/forward_list/debug/iterator2_neg.cc: New test.
>>>
>>> Tested under Linux x86_64 _GLIBCXX_DEBUG mode.
>>>
>>> Ok to commit ?
>>>
>>> François
>
>> diff --git a/libstdc++-v3/src/c++11/debug.cc b/libstdc++-v3/src/c++11/debug.cc
>> index 4706defedf1..cf8e6f48081 100644
>> --- a/libstdc++-v3/src/c++11/debug.cc
>> +++ b/libstdc++-v3/src/c++11/debug.cc
>> @@ -426,7 +426,8 @@ namespace __gnu_debug
>>    _M_reset() throw ()
>>    {
>>      __atomic_store_n(&_M_sequence, (_Safe_sequence_base*)0, __ATOMIC_RELEASE);
>> -    _M_version = 0;
>> +    // Detach iterator shall not look like a value-initialized one.
>> +    // _M_version = 0;
>>      _M_prior = 0;
>>      _M_next = 0;
>>    }
> I think this would be clearer as "Do not reset version, so that a
> detached iterator does not look like a value-initialized one."
>
>> +// { dg-do run { xfail *-*-* } }
>> +// { dg-require-debug-mode "" }
>> +
>> +#include <deque>
>> +
>> +#include <testsuite_hooks.h>
>> +
>> +void test01()
>> +{
>> +  typedef typename std::deque<int>::iterator It;
>> +  std::deque<int> dq;
>> +  dq.push_back(1);
>> +
>> +  It it = It();
>> +  VERIFY( dq.begin() != it );
> Is there any reason to use VERIFY here?
Only make sure the compiler do not optimize this check away.
>
> We're expecting the comparison to abort in the debug mode checks,
> right? Which would happen if we just do:
>
> (void) dq.begin() == it;

I guess this (void) cast is doing the same so adopted.

> Using VERIFY just makes it look like we're expecting the test to be
> XFAIL because the assertion will fail, but that's not what is being
> tested.
>
> OK for trunk with those changes, thanks.
>
Updated committed patch attached.