From: Jonathan Wakely <jwakely@redhat.com>
To: "François Dumont" <frs.dumont@gmail.com>
Cc: "libstdc++@gcc.gnu.org" <libstdc++@gcc.gnu.org>,
gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [Bug libstdc++/62313] Data race in debug iterators
Date: Sun, 21 Sep 2014 22:04:00 -0000 [thread overview]
Message-ID: <20140921220411.GD22778@redhat.com> (raw)
In-Reply-To: <5410BAB5.90803@gmail.com>
On 10/09/14 22:55 +0200, François Dumont wrote:
>Hi
>
> Here is a proposal to fix this data race issue.
>
> I finally generalized bitset approach to fix it by inheriting from
>the normal iterator first and then the _Safe_iterator_base type. None
>of the libstdc++ iterator types are final so it is fine. Surprisingly,
>despite inheritance being private gcc got confused between
>_Safe_iterator_base _M_next and forward_list _M_next so I need to
>adapt some code to make usage of _Safe_iterator_base _M_next explicit.
Access control in C++ is not related to visibility, name lookup still
finds private members, but it is an error to use them.
> I also consider any operator where normal iterator is being
>modified while the safe iterator is linked to the list of iterators.
>This is necessary to make sure that thread sanatizer won't consider a
>race condition. I didn't touch to bitset::reference because list
>references are only access on bitset destruction which is clearly not
>an operation allowed to do while playing with references in another
>thread.
>
> Do you see any way to check for this problem in the testsuite ? Is
>there a thread sanitizer we could use ?
GCC's -fsanitize=thread option, although using it in the testsuite
would need something like dg-require-tsan so the test doesn't run on
platforms where it doesn't work, or if GCC was built without
libsanitizer.
Have you run some tests using -fsanitize=thread, even if they are not
in the testsuite?
>Index: include/debug/safe_iterator.h
>===================================================================
>--- include/debug/safe_iterator.h (revision 215134)
>+++ include/debug/safe_iterator.h (working copy)
>@@ -109,16 +109,21 @@
> * %_Safe_iterator has member functions for iterator invalidation,
> * attaching/detaching the iterator from sequences, and querying
> * the iterator's state.
>+ *
>+ * Note that _Iterator must rely first in the type memory layout so that it
I can't parse this ... maybe "_Iterator must be the first base class" ?
>+ * gets initialized before the iterator is being attached to the container
s/container/container's/
>+ * list of iterators and it is being dettached before _Iterator get
s/dettached/detached/
>+ * destroy. Otherwise it would result in a data race.
s/destroy/destroyed/
> */
> template<typename _Iterator, typename _Sequence>
>- class _Safe_iterator : public _Safe_iterator_base
>+ class _Safe_iterator
>+ : private _Iterator,
>+ public _Safe_iterator_base
> {
>- typedef _Safe_iterator _Self;
>+ typedef _Iterator _Ite_base;
Please rename this to _Iter_base, "iter" is a more conventional
abbreviation than "ite"
>@@ -388,28 +433,27 @@
> /**
> * @brief Return the underlying iterator
> */
>- _Iterator
>- base() const _GLIBCXX_NOEXCEPT { return _M_current; }
>+ _Iterator&
>+ base() _GLIBCXX_NOEXCEPT { return *this; }
>
>+ const _Iterator&
>+ base() const _GLIBCXX_NOEXCEPT { return *this; }
I suppose base() doesn't need to be uglified to _M_base, because all
the containers already use std::reverse_iterator which uses the name
"base".
>Index: include/debug/safe_local_iterator.h
>===================================================================
>--- include/debug/safe_local_iterator.h (revision 215134)
>+++ include/debug/safe_local_iterator.h (working copy)
>@@ -49,15 +49,15 @@
> * the iterator's state.
> */
> template<typename _Iterator, typename _Sequence>
>- class _Safe_local_iterator : public _Safe_local_iterator_base
>+ class _Safe_local_iterator
>+ : private _Iterator
>+ , public _Safe_local_iterator_base
> {
>- typedef _Safe_local_iterator _Self;
>+ typedef _Iterator _Ite_base;
Same renaming here please, to _Iter_base.
Apart from those minor adjustments I think this looks good, but I'd
like to know that it has been tested with -fsanitize=thread, even if
only lightly tested.
next prev parent reply other threads:[~2014-09-21 22:04 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <bug-62313-19885@http.gcc.gnu.org/bugzilla/>
[not found] ` <bug-62313-19885-GBoNocJyMm@http.gcc.gnu.org/bugzilla/>
2014-09-10 20:55 ` François Dumont
2014-09-21 22:04 ` Jonathan Wakely [this message]
2014-09-23 20:42 ` François Dumont
2014-09-23 20:46 ` Marek Polacek
2014-09-25 22:01 ` François Dumont
2014-09-26 10:05 ` Jonathan Wakely
2014-09-30 15:32 ` Jonathan Wakely
2014-09-30 18:02 ` François Dumont
2014-09-30 20:18 ` François Dumont
2014-10-01 12:13 ` 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=20140921220411.GD22778@redhat.com \
--to=jwakely@redhat.com \
--cc=frs.dumont@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--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).