[Bug libstdc++/62313] Data race in debug iterators

[Jonathan Wakely]

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.