| Message ID | 2af7879c-bef5-7874-b5ae-722ad3a1e171@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 19/09/16 21:56 +0200, François Dumont wrote: >Hi > > Following our conversation here is a much simpler patch. I just >consider that all debug containers will have the same alignment. > > Even if I submit this patch as a whole I will commit into pieces, >at least one for the pure cleanup parts and one for the debug.cc >change. > > Among those changes there is: >- __gnu_cxx::__scoped_lock(this->_M_get_mutex()); >+ __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); > > I would appreciate if you could tell me what was happening with >the initial expression. I don't understand why it is compiling. I even >tried the same in debug.cc and started having compilation errors. It creates a temporary __scoped_lock, which immediately goes out of scope and unlocks the mutex again. This should be fixed on the gcc-5 and gcc-6 branches too. I'll review the rest of the patch today.
On 19/09/16 21:56 +0200, François Dumont wrote: >Hi > > Following our conversation here is a much simpler patch. I just >consider that all debug containers will have the same alignment. > > Even if I submit this patch as a whole I will commit into pieces, >at least one for the pure cleanup parts and one for the debug.cc >change. > > Among those changes there is: >- __gnu_cxx::__scoped_lock(this->_M_get_mutex()); >+ __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); > > I would appreciate if you could tell me what was happening with >the initial expression. I don't understand why it is compiling. I even >tried the same in debug.cc and started having compilation errors. > > * include/debug/bitset (bitset::reference::reference(const _Base_ref&, > bitset*)): Remove __unused__ attribute. > * include/debug/safe_base.h (_Safe_iterator_base): Make > _Safe_sequence_base a friend. > (_Safe_iterator_base::_M_attach): Make protected. > (_Safe_iterator_base::_M_attach_single): Likewise. > (_Safe_iterator_base::_M_detach): Likewise. > (_Safe_iterator_base::_M_detach_single): Likewise. > (_Safe_sequence_base): Make _Safe_iterator_base a friend. >(_Safe_sequence_base::_Safe_sequence_base(_Safe_sequence_base&&)): New. > (_Safe_sequence_base::_M_swap): Make protected. > (_Safe_sequence_base::_M_attach): Make private. > (_Safe_sequence_base::_M_attach_single): Likewise. > (_Safe_sequence_base::_M_detach): Likewise. > (_Safe_sequence_base::_M_detach_single): Likewise. > * include/debug/safe_container.h > (_Safe_container::_Safe_container(_Safe_container&&)): Make default. > * include/debug/safe_iterator.h > (_Safe_iterator::operator++()): Name __scoped_lock instance. > * include/debug/safe_iterator.tcc: Remove trailing line. > * include/debug/safe_unordered_base.h > (_Safe_local_iterator_base::_M_attach): Make protected. > (_Safe_local_iterator_base::_M_attach_single): Likewise. > (_Safe_local_iterator_base::_M_detach): Likewise. > (_Safe_local_iterator_base::_M_detach_single): Likewise. > (_Safe_unordered_container_base): Make _Safe_local_iterator_base >friend. > (_Safe_unordered_container_base::_M_attach_local): Make private. > (_Safe_unordered_container_base::_M_attach_local_single): Likewise. > (_Safe_unordered_container_base::_M_detach_local): Likewise. > (_Safe_unordered_container_base::_M_detach_local_single): Likewise. > * src/c++11/debug.cc: Include debug/vector. Include cctype. Remove > functional. > (get_safe_base_mutex): Get mutex based on address lowest non nil bits. > * testsuite/23_containers/vector/debug/mutex_association.cc: New. > >Tested under Linux x86_64. > >Ok to commit ? Yes, OK for trunk. Thanks for revising it, I think this is a much bettter fix. >On 15/09/2016 10:51, Jonathan Wakely wrote: >>N.B. https://gcc.gnu.org/PR71312 >>>Ok, debug mode is also impacted. Shouldn't the alignment be set on >>>the __mutex type directly ? >> >>No, definitely not. Apart from the fact that it would be an ABI >>change, it would mean that struct X { __mutex mx; int i; } would not >>place the int right next to the mutex, it would force it onto a >>different cacheline. We want our arrays of mutexes to be on separate >>cachelines, but most uses of __mutex are not in an array. >> >>We probably can't fix this properly yet, because we don't have the >>hardware_destructive_interference_size value. We could just make a >>conservative estimate of 64 bytes though. >Thanks for explaining that it is to avoid false sharing. > >Maybe we could share this mutex pool with debug mode. This way the day >we fix this false sharing issue it will benefit to both shared_ptr and >debug mode. Yes, that would reduce the size of the library, by not having two arrays of mutexes. Currently the arrays are not visible outside their own file, but that's not too hard to solve.
On 20/09/16 09:57 +0100, Jonathan Wakely wrote: >On 19/09/16 21:56 +0200, François Dumont wrote: >>Hi >> >> Following our conversation here is a much simpler patch. I just >>consider that all debug containers will have the same alignment. >> >> Even if I submit this patch as a whole I will commit into pieces, >>at least one for the pure cleanup parts and one for the debug.cc >>change. >> >> Among those changes there is: >>- __gnu_cxx::__scoped_lock(this->_M_get_mutex()); >>+ __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); >> >> I would appreciate if you could tell me what was happening with >>the initial expression. I don't understand why it is compiling. I >>even tried the same in debug.cc and started having compilation >>errors. >> >> * include/debug/bitset (bitset::reference::reference(const _Base_ref&, >> bitset*)): Remove __unused__ attribute. >> * include/debug/safe_base.h (_Safe_iterator_base): Make >> _Safe_sequence_base a friend. >> (_Safe_iterator_base::_M_attach): Make protected. >> (_Safe_iterator_base::_M_attach_single): Likewise. >> (_Safe_iterator_base::_M_detach): Likewise. >> (_Safe_iterator_base::_M_detach_single): Likewise. >> (_Safe_sequence_base): Make _Safe_iterator_base a friend. >>(_Safe_sequence_base::_Safe_sequence_base(_Safe_sequence_base&&)): New. >> (_Safe_sequence_base::_M_swap): Make protected. >> (_Safe_sequence_base::_M_attach): Make private. >> (_Safe_sequence_base::_M_attach_single): Likewise. >> (_Safe_sequence_base::_M_detach): Likewise. >> (_Safe_sequence_base::_M_detach_single): Likewise. >> * include/debug/safe_container.h >> (_Safe_container::_Safe_container(_Safe_container&&)): Make default. >> * include/debug/safe_iterator.h >> (_Safe_iterator::operator++()): Name __scoped_lock instance. >> * include/debug/safe_iterator.tcc: Remove trailing line. >> * include/debug/safe_unordered_base.h >> (_Safe_local_iterator_base::_M_attach): Make protected. >> (_Safe_local_iterator_base::_M_attach_single): Likewise. >> (_Safe_local_iterator_base::_M_detach): Likewise. >> (_Safe_local_iterator_base::_M_detach_single): Likewise. >> (_Safe_unordered_container_base): Make _Safe_local_iterator_base >>friend. >> (_Safe_unordered_container_base::_M_attach_local): Make private. >> (_Safe_unordered_container_base::_M_attach_local_single): Likewise. >> (_Safe_unordered_container_base::_M_detach_local): Likewise. >> (_Safe_unordered_container_base::_M_detach_local_single): Likewise. >> * src/c++11/debug.cc: Include debug/vector. Include cctype. Remove >> functional. >> (get_safe_base_mutex): Get mutex based on address lowest non nil bits. >> * testsuite/23_containers/vector/debug/mutex_association.cc: New. >> >>Tested under Linux x86_64. >> >>Ok to commit ? > >Yes, OK for trunk. Thanks for revising it, I think this is a much >bettter fix. This caused a new FAIL: FAIL: 23_containers/list/debug/invalidation/4.cc (test for excess errors) UNRESOLVED: 23_containers/list/debug/invalidation/4.cc compilation failed to produce executable
FAIL: 23_containers/list/debug/invalidation/4.cc (test for excess errors) Excess errors: /daten/aranym/gcc/gcc-20160926/Build/m68k-linux/libstdc++-v3/include/debug/safe_sequence.tcc:89: error: 'void __gnu_debug::_Safe_iterator_base::_M_detach_single()' is protected within this context /daten/aranym/gcc/gcc-20160926/Build/m68k-linux/libstdc++-v3/include/debug/safe_sequence.tcc:112: error: 'void __gnu_debug::_Safe_iterator_base::_M_detach_single()' is protected within this context Andreas.
On Sep 19 2016, François Dumont <frs.dumont@gmail.com> wrote: > diff --git a/libstdc++-v3/testsuite/23_containers/vector/debug/mutex_association.cc b/libstdc++-v3/testsuite/23_containers/vector/debug/mutex_association.cc > new file mode 100644 > index 0000000..a3c56e2 > --- /dev/null > +++ b/libstdc++-v3/testsuite/23_containers/vector/debug/mutex_association.cc > @@ -0,0 +1,42 @@ > +// Copyright (C) 2016 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/>. > +// > + > +#include <set> > +#include <debug/vector> > + > +#include <testsuite_hooks.h> > + > +class container : public __gnu_debug::_Safe_sequence<container> > +{ > +public: > + __gnu_cxx::__mutex& > + get_mutex() > + { return this->_M_get_mutex(); } > +}; > + > +int > +main() > +{ > + std::set<__gnu_cxx::__mutex*> mutexes; > + container conts[17]; > + > + for (int i = 0; i != 16; ++i) > + VERIFY( mutexes.insert(&conts[i].get_mutex()).second ); There will be less than 16 unique mutexes, if sizeof(container) has more trailing zero bits than alignof(__gnu_debug::vector<int>). Andreas.
diff --git a/libstdc++-v3/include/debug/bitset b/libstdc++-v3/include/debug/bitset index 55d3281..b7bada3 100644 --- a/libstdc++-v3/include/debug/bitset +++ b/libstdc++-v3/include/debug/bitset @@ -66,8 +66,7 @@ namespace __debug friend class bitset; reference(); - reference(const _Base_ref& __base, - bitset* __seq __attribute__((__unused__))) _GLIBCXX_NOEXCEPT + reference(const _Base_ref& __base, bitset* __seq) _GLIBCXX_NOEXCEPT : _Base_ref(__base) , _Safe_iterator_base(__seq, false) { } @@ -81,7 +80,7 @@ namespace __debug reference& operator=(bool __x) _GLIBCXX_NOEXCEPT { - _GLIBCXX_DEBUG_VERIFY(! this->_M_singular(), + _GLIBCXX_DEBUG_VERIFY(!this->_M_singular(), _M_message(__gnu_debug::__msg_bad_bitset_write) ._M_iterator(*this)); *static_cast<_Base_ref*>(this) = __x; @@ -91,10 +90,10 @@ namespace __debug reference& operator=(const reference& __x) _GLIBCXX_NOEXCEPT { - _GLIBCXX_DEBUG_VERIFY(! __x._M_singular(), + _GLIBCXX_DEBUG_VERIFY(!__x._M_singular(), _M_message(__gnu_debug::__msg_bad_bitset_read) ._M_iterator(__x)); - _GLIBCXX_DEBUG_VERIFY(! this->_M_singular(), + _GLIBCXX_DEBUG_VERIFY(!this->_M_singular(), _M_message(__gnu_debug::__msg_bad_bitset_write) ._M_iterator(*this)); *static_cast<_Base_ref*>(this) = __x; @@ -104,7 +103,7 @@ namespace __debug bool operator~() const _GLIBCXX_NOEXCEPT { - _GLIBCXX_DEBUG_VERIFY(! this->_M_singular(), + _GLIBCXX_DEBUG_VERIFY(!this->_M_singular(), _M_message(__gnu_debug::__msg_bad_bitset_read) ._M_iterator(*this)); return ~(*static_cast<const _Base_ref*>(this)); @@ -112,7 +111,7 @@ namespace __debug operator bool() const _GLIBCXX_NOEXCEPT { - _GLIBCXX_DEBUG_VERIFY(! this->_M_singular(), + _GLIBCXX_DEBUG_VERIFY(!this->_M_singular(), _M_message(__gnu_debug::__msg_bad_bitset_read) ._M_iterator(*this)); return *static_cast<const _Base_ref*>(this); @@ -121,7 +120,7 @@ namespace __debug reference& flip() _GLIBCXX_NOEXCEPT { - _GLIBCXX_DEBUG_VERIFY(! this->_M_singular(), + _GLIBCXX_DEBUG_VERIFY(!this->_M_singular(), _M_message(__gnu_debug::__msg_bad_bitset_flip) ._M_iterator(*this)); _Base_ref::flip(); diff --git a/libstdc++-v3/include/debug/safe_base.h b/libstdc++-v3/include/debug/safe_base.h index aad9c52..abdee73 100644 --- a/libstdc++-v3/include/debug/safe_base.h +++ b/libstdc++-v3/include/debug/safe_base.h @@ -49,6 +49,8 @@ namespace __gnu_debug */ class _Safe_iterator_base { + friend class _Safe_sequence_base; + public: /** The sequence this iterator references; may be NULL to indicate a singular iterator. */ @@ -101,7 +103,6 @@ namespace __gnu_debug __gnu_cxx::__mutex& _M_get_mutex() throw (); - public: /** Attaches this iterator to the given sequence, detaching it * from whatever sequence it was attached to originally. If the * new sequence is the NULL pointer, the iterator is left @@ -124,6 +125,7 @@ namespace __gnu_debug void _M_detach_single() throw (); + public: /** Determines if we are attached to the given sequence. */ bool _M_attached_to(const _Safe_sequence_base* __seq) const @@ -193,6 +195,8 @@ namespace __gnu_debug */ class _Safe_sequence_base { + friend class _Safe_iterator_base; + public: /// The list of mutable iterators that reference this container _Safe_iterator_base* _M_iterators; @@ -212,6 +216,11 @@ namespace __gnu_debug #if __cplusplus >= 201103L _Safe_sequence_base(const _Safe_sequence_base&) noexcept : _Safe_sequence_base() { } + + // Move constructor swap iterators. + _Safe_sequence_base(_Safe_sequence_base&& __seq) noexcept + : _Safe_sequence_base() + { _M_swap(__seq); } #endif /** Notify all iterators that reference this sequence that the @@ -250,12 +259,12 @@ namespace __gnu_debug __gnu_cxx::__mutex& _M_get_mutex() throw (); - public: /** Invalidates all iterators. */ void _M_invalidate_all() const { if (++_M_version == 0) _M_version = 1; } + private: /** Attach an iterator to this sequence. */ void _M_attach(_Safe_iterator_base* __it, bool __constant); diff --git a/libstdc++-v3/include/debug/safe_container.h b/libstdc++-v3/include/debug/safe_container.h index b73f68c..d96cb17 100644 --- a/libstdc++-v3/include/debug/safe_container.h +++ b/libstdc++-v3/include/debug/safe_container.h @@ -55,12 +55,9 @@ namespace __gnu_debug #if __cplusplus >= 201103L _Safe_container() = default; _Safe_container(const _Safe_container&) = default; - _Safe_container(_Safe_container&& __x) noexcept - : _Safe_container() - { _Base::_M_swap(__x); } + _Safe_container(_Safe_container&&) = default; - _Safe_container(_Safe_container&& __x, - const _Alloc& __a) + _Safe_container(_Safe_container&& __x, const _Alloc& __a) : _Safe_container() { if (__x._M_cont().get_allocator() == __a) diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h index c95d36c..41bc93a 100644 --- a/libstdc++-v3/include/debug/safe_iterator.h +++ b/libstdc++-v3/include/debug/safe_iterator.h @@ -295,7 +295,7 @@ namespace __gnu_debug _GLIBCXX_DEBUG_VERIFY(this->_M_incrementable(), _M_message(__msg_bad_inc) ._M_iterator(*this, "this")); - __gnu_cxx::__scoped_lock(this->_M_get_mutex()); + __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); ++base(); return *this; } diff --git a/libstdc++-v3/include/debug/safe_iterator.tcc b/libstdc++-v3/include/debug/safe_iterator.tcc index e8e1b00..0ae7fd1 100644 --- a/libstdc++-v3/include/debug/safe_iterator.tcc +++ b/libstdc++-v3/include/debug/safe_iterator.tcc @@ -93,4 +93,3 @@ namespace __gnu_debug } // namespace __gnu_debug #endif - diff --git a/libstdc++-v3/include/debug/safe_unordered_base.h b/libstdc++-v3/include/debug/safe_unordered_base.h index 82a42eb..21292c3 100644 --- a/libstdc++-v3/include/debug/safe_unordered_base.h +++ b/libstdc++-v3/include/debug/safe_unordered_base.h @@ -76,24 +76,27 @@ namespace __gnu_debug _Safe_unordered_container_base* _M_get_container() const noexcept; - public: /** Attaches this iterator to the given container, detaching it * from whatever container it was attached to originally. If the * new container is the NULL pointer, the iterator is left * unattached. */ - void _M_attach(_Safe_sequence_base* __seq, bool __constant); + void + _M_attach(_Safe_sequence_base* __seq, bool __constant); /** Likewise, but not thread-safe. */ - void _M_attach_single(_Safe_sequence_base* __seq, bool __constant) throw (); + void + _M_attach_single(_Safe_sequence_base* __seq, bool __constant) throw (); /** Detach the iterator for whatever container it is attached to, * if any. */ - void _M_detach(); + void + _M_detach(); /** Likewise, but not thread-safe. */ - void _M_detach_single() throw (); + void + _M_detach_single() throw (); }; /** @@ -116,7 +119,9 @@ namespace __gnu_debug */ class _Safe_unordered_container_base : public _Safe_sequence_base { + friend class _Safe_local_iterator_base; typedef _Safe_sequence_base _Base; + public: /// The list of mutable local iterators that reference this container _Safe_iterator_base* _M_local_iterators; @@ -158,7 +163,7 @@ namespace __gnu_debug void _M_swap(_Safe_unordered_container_base& __x) noexcept; - public: + private: /** Attach an iterator to this container. */ void _M_attach_local(_Safe_iterator_base* __it, bool __constant); diff --git a/libstdc++-v3/src/c++11/debug.cc b/libstdc++-v3/src/c++11/debug.cc index ed0417a..6b0db11 100644 --- a/libstdc++-v3/src/c++11/debug.cc +++ b/libstdc++-v3/src/c++11/debug.cc @@ -30,12 +30,13 @@ #include <debug/safe_unordered_base.h> #include <debug/safe_iterator.h> #include <debug/safe_local_iterator.h> +#include <debug/vector> #include <cassert> #include <cstdio> +#include <cctype> // for std::isspace #include <algorithm> // for std::min -#include <functional> // for _Hash_impl #include <cxxabi.h> // for __cxa_demangle @@ -47,11 +48,16 @@ namespace * in order to limit contention without breaking current library binary * compatibility. */ __gnu_cxx::__mutex& - get_safe_base_mutex(void* __address) + get_safe_base_mutex(void* address) { const size_t mask = 0xf; static __gnu_cxx::__mutex safe_base_mutex[mask + 1]; - const size_t index = _Hash_impl::hash(__address) & mask; + + // Use arbitrarily __gnu_debug::vector<int> as the container giving + // alignment of debug containers. + const auto alignbits = __builtin_ctz(alignof(__gnu_debug::vector<int>)); + const size_t index + = (reinterpret_cast<std::size_t>(address) >> alignbits) & mask; return safe_base_mutex[index]; } @@ -70,8 +76,8 @@ namespace } void - swap_seq(__gnu_debug::_Safe_sequence_base& __lhs, - __gnu_debug::_Safe_sequence_base& __rhs) + swap_seq_single(__gnu_debug::_Safe_sequence_base& __lhs, + __gnu_debug::_Safe_sequence_base& __rhs) { swap(__lhs._M_version, __rhs._M_version); swap_its(__lhs, __lhs._M_iterators, @@ -80,11 +86,42 @@ namespace __rhs, __rhs._M_const_iterators); } + template<typename _Action> + void + lock_and_run(__gnu_cxx::__mutex& lhs_mutex, __gnu_cxx::__mutex& rhs_mutex, + _Action action) + { + // We need to lock both sequences to run action. + if (&lhs_mutex == &rhs_mutex) + { + __gnu_cxx::__scoped_lock sentry(lhs_mutex); + action(); + } + else + { + __gnu_cxx::__scoped_lock sentry1(&lhs_mutex < &rhs_mutex + ? lhs_mutex : rhs_mutex); + __gnu_cxx::__scoped_lock sentry2(&lhs_mutex < &rhs_mutex + ? rhs_mutex : lhs_mutex); + action(); + } + } + + void + swap_seq(__gnu_cxx::__mutex& lhs_mutex, + __gnu_debug::_Safe_sequence_base& lhs, + __gnu_cxx::__mutex& rhs_mutex, + __gnu_debug::_Safe_sequence_base& rhs) + { + lock_and_run(lhs_mutex, rhs_mutex, + [&lhs, &rhs]() { swap_seq_single(lhs, rhs); }); + } + void - swap_ucont(__gnu_debug::_Safe_unordered_container_base& __lhs, - __gnu_debug::_Safe_unordered_container_base& __rhs) + swap_ucont_single(__gnu_debug::_Safe_unordered_container_base& __lhs, + __gnu_debug::_Safe_unordered_container_base& __rhs) { - swap_seq(__lhs, __rhs); + swap_seq_single(__lhs, __rhs); swap_its(__lhs, __lhs._M_local_iterators, __rhs, __rhs._M_local_iterators); swap_its(__lhs, __lhs._M_const_local_iterators, @@ -92,6 +129,16 @@ namespace } void + swap_ucont(__gnu_cxx::__mutex& lhs_mutex, + __gnu_debug::_Safe_unordered_container_base& lhs, + __gnu_cxx::__mutex& rhs_mutex, + __gnu_debug::_Safe_unordered_container_base& rhs) + { + lock_and_run(lhs_mutex, rhs_mutex, + [&lhs, &rhs]() { swap_ucont_single(lhs, rhs); }); + } + + void detach_all(__gnu_debug::_Safe_iterator_base* __iter) { for (; __iter;) @@ -248,25 +295,7 @@ namespace __gnu_debug void _Safe_sequence_base:: _M_swap(_Safe_sequence_base& __x) noexcept - { - // We need to lock both sequences to swap - using namespace __gnu_cxx; - __mutex *__this_mutex = &_M_get_mutex(); - __mutex *__x_mutex = &__x._M_get_mutex(); - if (__this_mutex == __x_mutex) - { - __scoped_lock __lock(*__this_mutex); - swap_seq(*this, __x); - } - else - { - __scoped_lock __l1(__this_mutex < __x_mutex - ? *__this_mutex : *__x_mutex); - __scoped_lock __l2(__this_mutex < __x_mutex - ? *__x_mutex : *__this_mutex); - swap_seq(*this, __x); - } - } + { swap_seq(_M_get_mutex(), *this, __x._M_get_mutex(), __x); } __gnu_cxx::__mutex& _Safe_sequence_base:: @@ -390,7 +419,7 @@ namespace __gnu_debug __gnu_cxx::__mutex& _Safe_iterator_base:: _M_get_mutex() throw () - { return get_safe_base_mutex(_M_sequence); } + { return _M_sequence->_M_get_mutex(); } _Safe_unordered_container_base* _Safe_local_iterator_base:: @@ -468,25 +497,7 @@ namespace __gnu_debug void _Safe_unordered_container_base:: _M_swap(_Safe_unordered_container_base& __x) noexcept - { - // We need to lock both containers to swap - using namespace __gnu_cxx; - __mutex *__this_mutex = &_M_get_mutex(); - __mutex *__x_mutex = &__x._M_get_mutex(); - if (__this_mutex == __x_mutex) - { - __scoped_lock __lock(*__this_mutex); - swap_ucont(*this, __x); - } - else - { - __scoped_lock __l1(__this_mutex < __x_mutex - ? *__this_mutex : *__x_mutex); - __scoped_lock __l2(__this_mutex < __x_mutex - ? *__x_mutex : *__this_mutex); - swap_ucont(*this, __x); - } - } + { swap_ucont(_M_get_mutex(), *this, __x._M_get_mutex(), __x); } void _Safe_unordered_container_base:: diff --git a/libstdc++-v3/testsuite/23_containers/vector/debug/mutex_association.cc b/libstdc++-v3/testsuite/23_containers/vector/debug/mutex_association.cc new file mode 100644 index 0000000..a3c56e2 --- /dev/null +++ b/libstdc++-v3/testsuite/23_containers/vector/debug/mutex_association.cc @@ -0,0 +1,42 @@ +// Copyright (C) 2016 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/>. +// + +#include <set> +#include <debug/vector> + +#include <testsuite_hooks.h> + +class container : public __gnu_debug::_Safe_sequence<container> +{ +public: + __gnu_cxx::__mutex& + get_mutex() + { return this->_M_get_mutex(); } +}; + +int +main() +{ + std::set<__gnu_cxx::__mutex*> mutexes; + container conts[17]; + + for (int i = 0; i != 16; ++i) + VERIFY( mutexes.insert(&conts[i].get_mutex()).second ); + + VERIFY( !mutexes.insert(&conts[16].get_mutex()).second ); +}
Hi Following our conversation here is a much simpler patch. I just consider that all debug containers will have the same alignment. Even if I submit this patch as a whole I will commit into pieces, at least one for the pure cleanup parts and one for the debug.cc change. Among those changes there is: - __gnu_cxx::__scoped_lock(this->_M_get_mutex()); + __gnu_cxx::__scoped_lock __l(this->_M_get_mutex()); I would appreciate if you could tell me what was happening with the initial expression. I don't understand why it is compiling. I even tried the same in debug.cc and started having compilation errors. * include/debug/bitset (bitset::reference::reference(const _Base_ref&, bitset*)): Remove __unused__ attribute. * include/debug/safe_base.h (_Safe_iterator_base): Make _Safe_sequence_base a friend. (_Safe_iterator_base::_M_attach): Make protected. (_Safe_iterator_base::_M_attach_single): Likewise. (_Safe_iterator_base::_M_detach): Likewise. (_Safe_iterator_base::_M_detach_single): Likewise. (_Safe_sequence_base): Make _Safe_iterator_base a friend. (_Safe_sequence_base::_Safe_sequence_base(_Safe_sequence_base&&)): New. (_Safe_sequence_base::_M_swap): Make protected. (_Safe_sequence_base::_M_attach): Make private. (_Safe_sequence_base::_M_attach_single): Likewise. (_Safe_sequence_base::_M_detach): Likewise. (_Safe_sequence_base::_M_detach_single): Likewise. * include/debug/safe_container.h (_Safe_container::_Safe_container(_Safe_container&&)): Make default. * include/debug/safe_iterator.h (_Safe_iterator::operator++()): Name __scoped_lock instance. * include/debug/safe_iterator.tcc: Remove trailing line. * include/debug/safe_unordered_base.h (_Safe_local_iterator_base::_M_attach): Make protected. (_Safe_local_iterator_base::_M_attach_single): Likewise. (_Safe_local_iterator_base::_M_detach): Likewise. (_Safe_local_iterator_base::_M_detach_single): Likewise. (_Safe_unordered_container_base): Make _Safe_local_iterator_base friend. (_Safe_unordered_container_base::_M_attach_local): Make private. (_Safe_unordered_container_base::_M_attach_local_single): Likewise. (_Safe_unordered_container_base::_M_detach_local): Likewise. (_Safe_unordered_container_base::_M_detach_local_single): Likewise. * src/c++11/debug.cc: Include debug/vector. Include cctype. Remove functional. (get_safe_base_mutex): Get mutex based on address lowest non nil bits. * testsuite/23_containers/vector/debug/mutex_association.cc: New. Tested under Linux x86_64. Ok to commit ? On 15/09/2016 10:51, Jonathan Wakely wrote: > N.B. https://gcc.gnu.org/PR71312 >> Ok, debug mode is also impacted. Shouldn't the alignment be set on >> the __mutex type directly ? > > No, definitely not. Apart from the fact that it would be an ABI > change, it would mean that struct X { __mutex mx; int i; } would not > place the int right next to the mutex, it would force it onto a > different cacheline. We want our arrays of mutexes to be on separate > cachelines, but most uses of __mutex are not in an array. > > We probably can't fix this properly yet, because we don't have the > hardware_destructive_interference_size value. We could just make a > conservative estimate of 64 bytes though. Thanks for explaining that it is to avoid false sharing. Maybe we could share this mutex pool with debug mode. This way the day we fix this false sharing issue it will benefit to both shared_ptr and debug mode. François