From: "François Dumont" <frs.dumont@gmail.com>
To: Jonathan Wakely <jwakely@redhat.com>
Cc: "libstdc++@gcc.gnu.org" <libstdc++@gcc.gnu.org>,
gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH][_GLIBCXX_DEBUG] Refine singular iterator state
Date: Mon, 8 Aug 2022 20:15:04 +0200 [thread overview]
Message-ID: <047769c5-7833-4b1f-787d-5de0a2755aa1@gmail.com> (raw)
In-Reply-To: <CACb0b4=pPscaB=Tn3SVLf-C5SmXRuEC7+wK0JP98hXEXe1yPvA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3608 bytes --]
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.
[-- Attachment #2: singular_value_init.patch --]
[-- Type: text/x-patch, Size: 15255 bytes --]
diff --git a/libstdc++-v3/include/debug/formatter.h b/libstdc++-v3/include/debug/formatter.h
index 80e8ba46d1e..748d4fbfea4 100644
--- a/libstdc++-v3/include/debug/formatter.h
+++ b/libstdc++-v3/include/debug/formatter.h
@@ -185,6 +185,7 @@ namespace __gnu_debug
__rbegin, // dereferenceable, and at the reverse-beginning
__rmiddle, // reverse-dereferenceable, not at the reverse-beginning
__rend, // reverse-past-the-end
+ __singular_value_init, // singular, value initialized
__last_state
};
@@ -280,7 +281,12 @@ namespace __gnu_debug
_M_variant._M_iterator._M_seq_type = _GLIBCXX_TYPEID(_Sequence);
if (__it._M_singular())
- _M_variant._M_iterator._M_state = __singular;
+ {
+ if (__it._M_value_initialized())
+ _M_variant._M_iterator._M_state = __singular_value_init;
+ else
+ _M_variant._M_iterator._M_state = __singular;
+ }
else
{
if (__it._M_is_before_begin())
@@ -308,7 +314,12 @@ namespace __gnu_debug
_M_variant._M_iterator._M_seq_type = _GLIBCXX_TYPEID(_Sequence);
if (__it._M_singular())
- _M_variant._M_iterator._M_state = __singular;
+ {
+ if (__it._M_value_initialized())
+ _M_variant._M_iterator._M_state = __singular_value_init;
+ else
+ _M_variant._M_iterator._M_state = __singular;
+ }
else
{
if (__it._M_is_end())
diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h
index d613933e236..33f7a86478a 100644
--- a/libstdc++-v3/include/debug/safe_iterator.h
+++ b/libstdc++-v3/include/debug/safe_iterator.h
@@ -41,8 +41,8 @@
#define _GLIBCXX_DEBUG_VERIFY_OPERANDS(_Lhs, _Rhs, _BadMsgId, _DiffMsgId) \
_GLIBCXX_DEBUG_VERIFY(!_Lhs._M_singular() && !_Rhs._M_singular() \
- || (_Lhs.base() == _Iterator() \
- && _Rhs.base() == _Iterator()), \
+ || (_Lhs._M_value_initialized() \
+ && _Rhs._M_value_initialized()), \
_M_message(_BadMsgId) \
._M_iterator(_Lhs, #_Lhs) \
._M_iterator(_Rhs, #_Rhs)); \
@@ -177,7 +177,7 @@ namespace __gnu_debug
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 408. Is vector<reverse_iterator<char*> > forbidden?
_GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
- || __x.base() == _Iterator(),
+ || __x._M_value_initialized(),
_M_message(__msg_init_copy_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
@@ -193,7 +193,7 @@ namespace __gnu_debug
: _Iter_base()
{
_GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
- || __x.base() == _Iterator(),
+ || __x._M_value_initialized(),
_M_message(__msg_init_copy_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
@@ -220,7 +220,7 @@ namespace __gnu_debug
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 408. Is vector<reverse_iterator<char*> > forbidden?
_GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
- || __x.base() == _MutableIterator(),
+ || __x._M_value_initialized(),
_M_message(__msg_init_const_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
@@ -236,7 +236,7 @@ namespace __gnu_debug
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 408. Is vector<reverse_iterator<char*> > forbidden?
_GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
- || __x.base() == _Iterator(),
+ || __x._M_value_initialized(),
_M_message(__msg_copy_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
@@ -266,7 +266,7 @@ namespace __gnu_debug
operator=(_Safe_iterator&& __x) noexcept
{
_GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
- || __x.base() == _Iterator(),
+ || __x._M_value_initialized(),
_M_message(__msg_copy_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
@@ -405,6 +405,11 @@ namespace __gnu_debug
_M_incrementable() const
{ return !this->_M_singular() && !_M_is_end(); }
+ /// Is the iterator value-initialized?
+ bool
+ _M_value_initialized() const
+ { return _M_version == 0 && base() == _Iter_base(); }
+
// Can we advance the iterator @p __n steps (@p __n may be negative)
bool
_M_can_advance(difference_type __n, bool __strict = false) const;
diff --git a/libstdc++-v3/include/debug/safe_local_iterator.h b/libstdc++-v3/include/debug/safe_local_iterator.h
index b5318f50b6a..6e3c4eb1505 100644
--- a/libstdc++-v3/include/debug/safe_local_iterator.h
+++ b/libstdc++-v3/include/debug/safe_local_iterator.h
@@ -33,8 +33,8 @@
#define _GLIBCXX_DEBUG_VERIFY_OPERANDS(_Lhs, _Rhs) \
_GLIBCXX_DEBUG_VERIFY(!_Lhs._M_singular() && !_Rhs._M_singular() \
- || (_Lhs.base() == _Iterator{} \
- && _Rhs.base() == _Iterator{}), \
+ || (_Lhs._M_value_initialized() \
+ && _Rhs._M_value_initialized()), \
_M_message(__msg_iter_compare_bad) \
._M_iterator(_Lhs, "lhs") \
._M_iterator(_Rhs, "rhs")); \
@@ -127,7 +127,7 @@ namespace __gnu_debug
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 408. Is vector<reverse_iterator<char*> > forbidden?
_GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
- || __x.base() == _Iterator(),
+ || __x._M_value_initialized(),
_M_message(__msg_init_copy_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
@@ -142,7 +142,7 @@ namespace __gnu_debug
: _Iter_base()
{
_GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
- || __x.base() == _Iterator(),
+ || __x._M_value_initialized(),
_M_message(__msg_init_copy_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
@@ -167,7 +167,7 @@ namespace __gnu_debug
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 408. Is vector<reverse_iterator<char*> > forbidden?
_GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
- || __x.base() == _MutableIterator(),
+ || __x._M_value_initialized(),
_M_message(__msg_init_const_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
@@ -183,7 +183,7 @@ namespace __gnu_debug
// _GLIBCXX_RESOLVE_LIB_DEFECTS
// DR 408. Is vector<reverse_iterator<char*> > forbidden?
_GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
- || __x.base() == _Iterator(),
+ || __x._M_value_initialized(),
_M_message(__msg_copy_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
@@ -212,7 +212,7 @@ namespace __gnu_debug
operator=(_Safe_local_iterator&& __x) noexcept
{
_GLIBCXX_DEBUG_VERIFY(!__x._M_singular()
- || __x.base() == _Iterator(),
+ || __x._M_value_initialized(),
_M_message(__msg_copy_singular)
._M_iterator(*this, "this")
._M_iterator(__x, "other"));
@@ -343,6 +343,11 @@ namespace __gnu_debug
_M_incrementable() const
{ return !this->_M_singular() && !_M_is_end(); }
+ /// Is the iterator value-initialized?
+ bool
+ _M_value_initialized() const
+ { return _M_version == 0 && base() == _Iter_base{}; }
+
// Is the iterator range [*this, __rhs) valid?
bool
_M_valid_range(const _Safe_local_iterator& __rhs,
diff --git a/libstdc++-v3/src/c++11/debug.cc b/libstdc++-v3/src/c++11/debug.cc
index 4706defedf1..8ed61a69913 100644
--- a/libstdc++-v3/src/c++11/debug.cc
+++ b/libstdc++-v3/src/c++11/debug.cc
@@ -426,7 +426,9 @@ namespace __gnu_debug
_M_reset() throw ()
{
__atomic_store_n(&_M_sequence, (_Safe_sequence_base*)0, __ATOMIC_RELEASE);
- _M_version = 0;
+ // Do not reset version, so that a detached iterator does not look like a
+ // value-initialized one.
+ // _M_version = 0;
_M_prior = 0;
_M_next = 0;
}
@@ -767,7 +769,8 @@ namespace
"before-begin",
"dereferenceable (start-of-reverse-sequence)",
"dereferenceable (reverse)",
- "past-the-reverse-end"
+ "past-the-reverse-end",
+ "singular (value-initialized)"
};
print_word(ctx, state_names[iterator._M_state]);
}
diff --git a/libstdc++-v3/testsuite/23_containers/deque/debug/iterator1_neg.cc b/libstdc++-v3/testsuite/23_containers/deque/debug/iterator1_neg.cc
new file mode 100644
index 00000000000..73f8a044d43
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/deque/debug/iterator1_neg.cc
@@ -0,0 +1,37 @@
+// Copyright (C) 2022 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+//
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <deque>
+
+void test01()
+{
+ typedef typename std::deque<int>::iterator It;
+ std::deque<int> dq;
+ dq.push_back(1);
+
+ It it = It();
+ (void)(dq.begin() != it);
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
diff --git a/libstdc++-v3/testsuite/23_containers/deque/debug/iterator2_neg.cc b/libstdc++-v3/testsuite/23_containers/deque/debug/iterator2_neg.cc
new file mode 100644
index 00000000000..0abf5cbd4ec
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/deque/debug/iterator2_neg.cc
@@ -0,0 +1,40 @@
+// Copyright (C) 2022 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+//
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <deque>
+
+void test01()
+{
+ typedef typename std::deque<int>::iterator It;
+ It it;
+ {
+ std::deque<int> dq;
+ it = dq.begin();
+ }
+
+ It value_init_it = It();
+ (void)(it != value_init_it);
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
diff --git a/libstdc++-v3/testsuite/23_containers/forward_list/debug/iterator1_neg.cc b/libstdc++-v3/testsuite/23_containers/forward_list/debug/iterator1_neg.cc
new file mode 100644
index 00000000000..8ca44e248ed
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/forward_list/debug/iterator1_neg.cc
@@ -0,0 +1,37 @@
+// Copyright (C) 2022 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+//
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <forward_list>
+
+void test01()
+{
+ typedef typename std::forward_list<int>::iterator It;
+ std::forward_list<int> fl;
+ fl.push_front(1);
+
+ It it = It();
+ (void)(fl.begin() != it);
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
diff --git a/libstdc++-v3/testsuite/23_containers/forward_list/debug/iterator2_neg.cc b/libstdc++-v3/testsuite/23_containers/forward_list/debug/iterator2_neg.cc
new file mode 100644
index 00000000000..92ab059e6b8
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/forward_list/debug/iterator2_neg.cc
@@ -0,0 +1,40 @@
+// Copyright (C) 2022 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+//
+// { dg-do run { target c++11 xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <forward_list>
+
+void test01()
+{
+ typedef typename std::forward_list<int>::iterator It;
+ It it;
+ {
+ std::forward_list<int> fl;
+ it = fl.begin();
+ }
+
+ It value_init_it{};
+ (void)(it != value_init_it);
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
diff --git a/libstdc++-v3/testsuite/23_containers/forward_list/debug/iterator3_neg.cc b/libstdc++-v3/testsuite/23_containers/forward_list/debug/iterator3_neg.cc
new file mode 100644
index 00000000000..32ae7a5b7a6
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/forward_list/debug/iterator3_neg.cc
@@ -0,0 +1,45 @@
+// Copyright (C) 2022 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+//
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <forward_list>
+
+#include <testsuite_hooks.h>
+
+void test01()
+{
+ typedef typename std::forward_list<int>::iterator It;
+ It end1, end2;
+
+ {
+ std::forward_list<int> fl;
+ fl.push_front(1);
+
+ end1 = end2 = fl.end();
+ VERIFY( end1 == end2 );
+ }
+
+ (void)(end1 == end2);
+}
+
+int main()
+{
+ test01();
+ return 0;
+}
next prev parent reply other threads:[~2022-08-08 18:15 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-04 20:56 François Dumont
2022-08-08 5:07 ` François Dumont
2022-08-08 13:19 ` Jonathan Wakely
2022-08-08 18:15 ` François Dumont [this message]
2022-08-08 22:26 ` Jonathan Wakely
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=047769c5-7833-4b1f-787d-5de0a2755aa1@gmail.com \
--to=frs.dumont@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jwakely@redhat.com \
--cc=libstdc++@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).