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

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 );
+}

Reply via email to