Re: [Bug libstdc++/62313] Data race in debug iterators
On 30/09/14 22:18 +0200, François Dumont wrote: Hi I prefer to submit this patch to you cause I am not very comfortable with Python stuff. I simply rely on Python cast feature. It doesn't really matter but is it going to simply consider the debug iterator as a normal one or Yes. is it going through the C++ explicit cast operator on debug iterators ? No, it doesn't call any C++ function. (N.B. I was searching in debug/safe_iterator.h for the 'explicit' conversion operator you referred to and was confused because I couldn't find it, which is because the operator for converting to base is not 'explicit') I'm not sure why the existing Python code does .cast(itype) when _M_current is already that type, that seems unnecessary to me, but I think your fix is correct and OK to commit, thanks.
Re: [Bug libstdc++/62313] Data race in debug iterators
On 26/09/14 11:05 +0100, Jonathan Wakely wrote: On 26/09/14 00:00 +0200, François Dumont wrote: 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. Hi Dmitry, who reported the bug, confirmed the fix. Can I go ahead and commit ? Yes, OK. This caused some failures in the printer tests: Running /home/jwakely/src/gcc/gcc/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp ... FAIL: libstdc++-prettyprinters/debug.cc print deqiter FAIL: libstdc++-prettyprinters/debug.cc print lstiter FAIL: libstdc++-prettyprinters/debug.cc print lstciter FAIL: libstdc++-prettyprinters/debug.cc print mpiter FAIL: libstdc++-prettyprinters/debug.cc print spciter
Re: [Bug libstdc++/62313] Data race in debug iterators
I forgot to check pretty printer tests indeed, I will. François On 30/09/2014 17:32, Jonathan Wakely wrote: On 26/09/14 11:05 +0100, Jonathan Wakely wrote: On 26/09/14 00:00 +0200, François Dumont wrote: 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. Hi Dmitry, who reported the bug, confirmed the fix. Can I go ahead and commit ? Yes, OK. This caused some failures in the printer tests: Running /home/jwakely/src/gcc/gcc/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp ... FAIL: libstdc++-prettyprinters/debug.cc print deqiter FAIL: libstdc++-prettyprinters/debug.cc print lstiter FAIL: libstdc++-prettyprinters/debug.cc print lstciter FAIL: libstdc++-prettyprinters/debug.cc print mpiter FAIL: libstdc++-prettyprinters/debug.cc print spciter
Re: [Bug libstdc++/62313] Data race in debug iterators
Hi I prefer to submit this patch to you cause I am not very comfortable with Python stuff. I simply rely on Python cast feature. It doesn't really matter but is it going to simply consider the debug iterator as a normal one or is it going through the C++ explicit cast operator on debug iterators ? François On 30/09/2014 17:32, Jonathan Wakely wrote: On 26/09/14 11:05 +0100, Jonathan Wakely wrote: On 26/09/14 00:00 +0200, François Dumont wrote: 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. Hi Dmitry, who reported the bug, confirmed the fix. Can I go ahead and commit ? Yes, OK. This caused some failures in the printer tests: Running /home/jwakely/src/gcc/gcc/libstdc++-v3/testsuite/libstdc++-prettyprinters/prettyprinters.exp ... FAIL: libstdc++-prettyprinters/debug.cc print deqiter FAIL: libstdc++-prettyprinters/debug.cc print lstiter FAIL: libstdc++-prettyprinters/debug.cc print lstciter FAIL: libstdc++-prettyprinters/debug.cc print mpiter FAIL: libstdc++-prettyprinters/debug.cc print spciter Index: python/libstdcxx/v6/printers.py === --- python/libstdcxx/v6/printers.py (revision 215741) +++ python/libstdcxx/v6/printers.py (working copy) @@ -460,7 +460,7 @@ # and return the wrapped iterator value. def to_string (self): itype = self.val.type.template_argument(0) -return self.val['_M_current'].cast(itype) +return self.val.cast(itype) class StdMapPrinter: Print a std::map or std::multimap
Re: [Bug libstdc++/62313] Data race in debug iterators
On 26/09/14 00:00 +0200, François Dumont wrote: 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. Hi Dmitry, who reported the bug, confirmed the fix. Can I go ahead and commit ? Yes, OK.
Re: [Bug libstdc++/62313] Data race in debug iterators
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. Hi Dmitry, who reported the bug, confirmed the fix. Can I go ahead and commit ? François
Re: [Bug libstdc++/62313] Data race in debug iterators
On 22/09/2014 00:04, Jonathan Wakely wrote: 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. Ok, tricky. 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? No I hadn't and try since but without success. When I build with -fsanitize=thread the produced binary just segfault at startup. It complained about using -fPIE at compilation time and -lpie at link time but even with those options it segfault. Don't know what is going wrong. Maybe Dmitry who reported the bug could give it a try. I will ask for this on the bug ticket Index: include/debug/safe_iterator.h 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. I fixed the vocabulary problems. Just need a slight test then. François
Re: [Bug libstdc++/62313] Data race in debug iterators
On Tue, Sep 23, 2014 at 10:42:31PM +0200, François Dumont wrote: No I hadn't and try since but without success. When I build with -fsanitize=thread the produced binary just segfault at startup. It complained about using -fPIE at compilation time and -lpie at link time but even with those options it segfault. Don't know what is going wrong. Maybe At link time it should be -pie, not -lpie. Marek
Re: [Bug libstdc++/62313] Data race in debug iterators
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/
*/
templatetypename _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.
*/
templatetypename _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.
[Bug libstdc++/62313] Data race in debug iterators
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.
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 ?
2014-09-10 François Dumont fdum...@gcc.gnu.org
PR libstdc++/62313
* include/debug/safe_base.h
(_Safe_iterator_base(const _Safe_iterator_base)): Delete declaration.
(_Safe_iterator_base operator=(const _Safe_iterator_base)): Likewise.
* include/debug/safe_iterator.h (_Safe_iterator): Move normal
iterator
before _Safe_iterator_base in memory. Lock before modifying the
iterator
in numerous places.
* include/debug/safe_local_iterator.h
(_Safe_local_iterator_base(const _Safe_local_iterator_base)): Delete
declaration.
(_Safe_local_iterator_base operator=(const
_Safe_local_iterator_base)):
Likewise.
* include/debug/safe_unordered_base.h (_Safe_local_iterator): Move
normal iterator before _Safe_iterator_base in memory. Lock before
modifying the iterator in numerous places.
* include/debug/forward_list (_Safe_forward_list::_M_swap_aux):
Adapt.
* include/debug/safe_sequence.tcc
(_Safe_sequence::_M_transfer_from_if): Adapt.
Tested under Linux x86_64 debug mode.
Ok to commit ?
François
Index: include/debug/forward_list
===
--- include/debug/forward_list (revision 215134)
+++ include/debug/forward_list (working copy)
@@ -86,24 +86,26 @@
for (_Safe_iterator_base* __iter = __lhs_iterators; __iter;)
{
// Even iterator is cast to const_iterator, not a problem.
- const_iterator* __victim = static_castconst_iterator*(__iter);
+ _Safe_iterator_base* __victim_base = __iter;
+ const_iterator* __victim =
+ static_castconst_iterator*(__victim_base);
__iter = __iter-_M_next;
if (__victim-base() == __rseq._M_base().cbefore_begin())
{
__victim-_M_unlink();
- if (__lhs_iterators == __victim)
- __lhs_iterators = __victim-_M_next;
+ if (__lhs_iterators == __victim_base)
+ __lhs_iterators = __victim_base-_M_next;
if (__bbegin_its)
{
- __victim-_M_next = __bbegin_its;
- __bbegin_its-_M_prior = __victim;
+ __victim_base-_M_next = __bbegin_its;
+ __bbegin_its-_M_prior = __victim_base;
}
else
- __last_bbegin = __victim;
- __bbegin_its = __victim;
+ __last_bbegin = __victim_base;
+ __bbegin_its = __victim_base;
}
else
- __victim-_M_sequence = __lhs;
+ __victim_base-_M_sequence = __lhs;
}
if (__bbegin_its)
Index: include/debug/safe_base.h
===
--- include/debug/safe_base.h (revision 215134)
+++ include/debug/safe_base.h (working copy)
@@ -95,12 +95,6 @@
: _M_sequence(0), _M_version(0), _M_prior(0), _M_next(0)
{ this-_M_attach(__x._M_sequence, __constant); }
-_Safe_iterator_base
-operator=(const _Safe_iterator_base);
-
-explicit
-_Safe_iterator_base(const _Safe_iterator_base);
-
~_Safe_iterator_base() { this-_M_detach(); }
/** For use in _Safe_iterator. */
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
+ * gets initialized before the iterator is being attached to the container
+ * list of iterators and it is being dettached before _Iterator get
+ * destroy. Otherwise it would result in a data race.
*/
templatetypename _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;
+ typedef _Safe_iterator_base _Safe_base;
typedef
