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

[Jonathan Wakely <jwakely at redhat dot 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.