Re: debug container mutex association

[Andreas Schwab]

On Sep 19 2016, François Dumont  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 000..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
> +// .
> +//
> +
> +#include 
> +#include 
> +
> +#include 
> +
> +class container : public __gnu_debug::_Safe_sequence
> +{
> +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([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).

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: debug container mutex association

[Jonathan Wakely]

On 28/09/16 21:30 +0200, François Dumont wrote:

On 27/09/2016 12:32, Jonathan Wakely wrote:

Index: include/debug/safe_base.h

===
--- include/debug/safe_base.h(revision 240509)
+++ include/debug/safe_base.h(working copy)
@@ -121,11 +121,11 @@
   void
   _M_detach();

+  public:
   /** Likewise, but not thread-safe. */
   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



Would this be a smaller change, that doesn't make the member
accessible to all code?

--- a/libstdc++-v3/include/debug/safe_base.h
+++ b/libstdc++-v3/include/debug/safe_base.h
@@ -50,6 +50,7 @@ namespace __gnu_debug
 class _Safe_iterator_base
 {
   friend class _Safe_sequence_base;
+template friend class _Safe_sequence;

 public:
   /** The sequence this iterator references; may be NULL to indicate
.

I am not a great fan of friend class. As long as it was friend 
declaration between iterator and sequence base types it was ok. Now 
that we need to make a template class friend I consider that it is too 
much friendship and prefer to make the necessary method public.


OK.


But if you think otherwise just tell me and I will use your approach.


Making it public is fine. Please commit your original patch, thanks!

Re: debug container mutex association

[François Dumont]

On 27/09/2016 12:32, Jonathan Wakely wrote:

Index: include/debug/safe_base.h

===
--- include/debug/safe_base.h(revision 240509)
+++ include/debug/safe_base.h(working copy)
@@ -121,11 +121,11 @@
void
_M_detach();

+  public:
/** Likewise, but not thread-safe. */
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



Would this be a smaller change, that doesn't make the member
accessible to all code?

--- a/libstdc++-v3/include/debug/safe_base.h
+++ b/libstdc++-v3/include/debug/safe_base.h
@@ -50,6 +50,7 @@ namespace __gnu_debug
  class _Safe_iterator_base
  {
friend class _Safe_sequence_base;
+template friend class _Safe_sequence;

  public:
/** The sequence this iterator references; may be NULL to indicate
.

I am not a great fan of friend class. As long as it was friend 
declaration between iterator and sequence base types it was ok. Now that 
we need to make a template class friend I consider that it is too much 
friendship and prefer to make the necessary method public.


But if you think otherwise just tell me and I will use your approach.

François

Re: debug container mutex association

[François Dumont]

On 27/09/2016 17:29, Jonathan Wakely wrote:

On 20/09/16 09:53 +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.


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'm committing this to the gcc-5 and gcc-6 branches.


I still had plan to do so but if you have those branches under your 
hands I appreciate.


Thanks,

François

Re: debug container mutex association

[Jonathan Wakely]

On 20/09/16 09:53 +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.


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'm committing this to the gcc-5 and gcc-6 branches.


commit 5218b515fdb54881d8a2b87c28eb5fd84c52db01
Author: Jonathan Wakely 
Date:   Tue Sep 27 16:22:28 2016 +0100

Fix lifetime of mutex lock in debug iterator

	* include/debug/safe_iterator.h (_Safe_iterator::operator++()): Fix
	lifetime of lock.

diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h
index 5368f3b..3f5a7f8 100644
--- a/libstdc++-v3/include/debug/safe_iterator.h
+++ b/libstdc++-v3/include/debug/safe_iterator.h
@@ -296,7 +296,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;
   }

Re: debug container mutex association

[Jonathan Wakely]

On 26/09/16 22:36 +0200, François Dumont wrote:

Fixed with attached patch.

François


On 26/09/2016 13:56, Andreas Schwab wrote:

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.






Index: include/debug/safe_base.h
===
--- include/debug/safe_base.h   (revision 240509)
+++ include/debug/safe_base.h   (working copy)
@@ -121,11 +121,11 @@
void
_M_detach();

+  public:
/** Likewise, but not thread-safe. */
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



Would this be a smaller change, that doesn't make the member
accessible to all code?

--- a/libstdc++-v3/include/debug/safe_base.h
+++ b/libstdc++-v3/include/debug/safe_base.h
@@ -50,6 +50,7 @@ namespace __gnu_debug
  class _Safe_iterator_base
  {
friend class _Safe_sequence_base;
+template friend class _Safe_sequence;

  public:
/** The sequence this iterator references; may be NULL to indicate

Re: debug container mutex association

[François Dumont]

Fixed with attached patch.

François


On 26/09/2016 13:56, Andreas Schwab wrote:

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.



Index: include/debug/safe_base.h
===
--- include/debug/safe_base.h	(revision 240509)
+++ include/debug/safe_base.h	(working copy)
@@ -121,11 +121,11 @@
 void
 _M_detach();
 
+  public:
 /** Likewise, but not thread-safe. */
 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

Re: debug container mutex association

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: debug container mutex association

[Jonathan Wakely]

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

Re: debug container mutex association

[Jonathan Wakely]

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.

Re: debug container mutex association

[Jonathan Wakely]

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.

Re: debug container mutex association

[François Dumont]

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
 	{
-	  

Re: debug container mutex association

[Jonathan Wakely]

On 14/09/16 22:17 +0200, François Dumont wrote:

On 14/09/2016 11:00, Jonathan Wakely wrote:

On 13/09/16 22:37 +0200, François Dumont wrote:

Hi

  When I proposed to change std::hash for pointers my plan was to 
use this approach for the debug containers. So here is the patch 
leveraging on this technique to avoid going through _Hash_impl to 
hash address and get mutex index from it. I think it is obious 
that new access is faster so I didn't wrote a performance test to 
demonstrate it.


Is this actually a bottleneck that needs to be made faster?

I know it's nice if Debug Mode isn't very slow, but you've already
made it much better, and performance is not the primary goal of Debug
Mode.


Sure but my approach is that if I can make something faster then I 
just try. I considered that making this mode faster will allow its 
usage in an even more extended way.




diff --git a/libstdc++-v3/config/abi/pre/gnu.ver 
b/libstdc++-v3/config/abi/pre/gnu.ver

index 9b5bb23..c9a253e 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -1947,6 +1947,21 @@ GLIBCXX_3.4.23 {
   _ZNSsC[12]ERKSs[jmy]RKSaIcE;
   _ZNSbIwSt11char_traitsIwESaIwEEC[12]ERKS2_mRKS1_;

+# __gnu_debug::_Safe_sequence_base
+ _ZN11__gnu_debug19_Safe_sequence_base18_M_detach_singularEm;


The 'm' here should be [jmy] because std::size_t mangles differently
on different targets.


+ _ZN11__gnu_debug19_Safe_sequence_base22_M_revalidate_singularEm;
+_ZN11__gnu_debug19_Safe_sequence_base12_M_get_mutexEm;
+_ZN11__gnu_debug19_Safe_sequence_base7_M_swapERS0_m;
+
+# __gnu_debug::_Safe_iterator_base
+ _ZN11__gnu_debug19_Safe_iterator_base9_M_attachEPNS_19_Safe_sequence_baseEbm;
+_ZN11__gnu_debug19_Safe_iterator_base9_M_detachEm;
+
+# __gnu_debug::_Safe_unordered_container_base and 
_Safe_local_iterator_base

+ _ZN11__gnu_debug30_Safe_unordered_container_base7_M_swapERS0_m;
+ 
_ZN11__gnu_debug25_Safe_local_iterator_base9_M_attachEPNS_19_Safe_sequence_baseEbm;
+_ZN11__gnu_debug25_Safe_local_iterator_base9_M_detachEm;
+
} GLIBCXX_3.4.22;


It's unfortunate to have to export and maintain new symbols when we
don't offer the same level of ABI guarantee for Debug Mode  :-(


Yes, I have been told that Debug ABI compatibility was not guaranteed 
but at the same time if we don't do so abi-check is failing. Couldn't 
we have a special section in gnu.ver for unversioned symbols like 
those ?


I we could, but making them unversioned wouldn't really be useful - we
still need to keep them in the library forever.


@@ -174,6 +191,18 @@ namespace __gnu_debug
   const _Safe_iterator_base* __last)
 { return __first->_M_can_compare(*__last); }

+  // Compute power of 2 not greater than __n.
+  template
+struct _Lowest_power_of_two
+{
+  static const size_t __val
+= _Lowest_power_of_two< (__n >> 1) >::__val + 1;
+};
+
+  template<>
+struct _Lowest_power_of_two<1>
+{ static const size_t __val = 1; };


The lowest power of two not greater than 1 is 2^0, but this gives the
result 1.

Similarly, the lowest power of two not greater than 2 is 2^1, i.e. 1.


Indeed, when writing the test case I decided to return 1 rather than 0 
for the _Lowest_power_of_two<1> specialization.It gave a better 
container instance - mutex mapping. But I forgot to change its name.




And the name is misleading, it doesn't compute a power of two, it
computes the base-2 logarithm (then adds one), so it's a compile-time
version of floor(log2(n))+1.


@@ -123,6 +125,36 @@ namespace __gnu_debug
 template
   void
   _M_transfer_from_if(_Safe_sequence& __from, _Predicate __pred);
+
+  static _GLIBCXX_CONSTEXPR std::size_t
+  _S_alignment() _GLIBCXX_NOEXCEPT
+  { return _Lowest_power_of_two<__alignof(_Sequence)>::__val; }


This function is called "alignment" but it returns log2(alignof(T))+1


Yes, I wasn't proud about the naming.



Does it need to be constexpr? Is that for std::array?


No, it is not used in std::array context. I just considered that it 
could be constexpr.


OK. Sometimes that's useful, I agree. But it's never used in a
constexpr context, and it's only called by the library directly, so
there's no advantage here (it just makes it harder to write because it
can only use code that's valid in a constant expression).

Anyway, you can write it without a template, and still be constexpr:



You can get the same result without _Lowest_power_of_two:

static _GLIBCXX_CONSTEXPR size_t
_S_alignbits() _GLIBCXX_NOEXCEPT
{ return __builtin_ctz(__alignof(_Sequence)) + 1; }

But I'm not convinced the +1 is correct, see below.


@@ -46,15 +47,30 @@ namespace
 /** Returns different instances of __mutex depending on the 
passed address
  *  in order to limit contention without breaking current 
library binary

  *  compatibility. */
+  const size_t mask = 0xf;
+
 __gnu_cxx::__mutex&
-  get_safe_base_mutex(void* __address)
+  get_mutex(size_t index)
 {
-const size_t mask = 0xf;
   

Re: debug container mutex association

[François Dumont]

On 14/09/2016 11:00, Jonathan Wakely wrote:

On 13/09/16 22:37 +0200, François Dumont wrote:

Hi

   When I proposed to change std::hash for pointers my plan was to 
use this approach for the debug containers. So here is the patch 
leveraging on this technique to avoid going through _Hash_impl to 
hash address and get mutex index from it. I think it is obious that 
new access is faster so I didn't wrote a performance test to 
demonstrate it.


Is this actually a bottleneck that needs to be made faster?

I know it's nice if Debug Mode isn't very slow, but you've already
made it much better, and performance is not the primary goal of Debug
Mode.


Sure but my approach is that if I can make something faster then I just 
try. I considered that making this mode faster will allow its usage in 
an even more extended way.




diff --git a/libstdc++-v3/config/abi/pre/gnu.ver 
b/libstdc++-v3/config/abi/pre/gnu.ver

index 9b5bb23..c9a253e 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -1947,6 +1947,21 @@ GLIBCXX_3.4.23 {
_ZNSsC[12]ERKSs[jmy]RKSaIcE;
_ZNSbIwSt11char_traitsIwESaIwEEC[12]ERKS2_mRKS1_;

+# __gnu_debug::_Safe_sequence_base
+ _ZN11__gnu_debug19_Safe_sequence_base18_M_detach_singularEm;


The 'm' here should be [jmy] because std::size_t mangles differently
on different targets.


+ _ZN11__gnu_debug19_Safe_sequence_base22_M_revalidate_singularEm;
+_ZN11__gnu_debug19_Safe_sequence_base12_M_get_mutexEm;
+_ZN11__gnu_debug19_Safe_sequence_base7_M_swapERS0_m;
+
+# __gnu_debug::_Safe_iterator_base
+ 
_ZN11__gnu_debug19_Safe_iterator_base9_M_attachEPNS_19_Safe_sequence_baseEbm;

+_ZN11__gnu_debug19_Safe_iterator_base9_M_detachEm;
+
+# __gnu_debug::_Safe_unordered_container_base and 
_Safe_local_iterator_base

+ _ZN11__gnu_debug30_Safe_unordered_container_base7_M_swapERS0_m;
+ 
_ZN11__gnu_debug25_Safe_local_iterator_base9_M_attachEPNS_19_Safe_sequence_baseEbm;

+_ZN11__gnu_debug25_Safe_local_iterator_base9_M_detachEm;
+
} GLIBCXX_3.4.22;


It's unfortunate to have to export and maintain new symbols when we
don't offer the same level of ABI guarantee for Debug Mode  :-(


Yes, I have been told that Debug ABI compatibility was not guaranteed 
but at the same time if we don't do so abi-check is failing. Couldn't we 
have a special section in gnu.ver for unversioned symbols like those ?





@@ -174,6 +191,18 @@ namespace __gnu_debug
const _Safe_iterator_base* __last)
  { return __first->_M_can_compare(*__last); }

+  // Compute power of 2 not greater than __n.
+  template
+struct _Lowest_power_of_two
+{
+  static const size_t __val
+= _Lowest_power_of_two< (__n >> 1) >::__val + 1;
+};
+
+  template<>
+struct _Lowest_power_of_two<1>
+{ static const size_t __val = 1; };


The lowest power of two not greater than 1 is 2^0, but this gives the
result 1.

Similarly, the lowest power of two not greater than 2 is 2^1, i.e. 1.


Indeed, when writing the test case I decided to return 1 rather than 0 
for the _Lowest_power_of_two<1> specialization.It gave a better 
container instance - mutex mapping. But I forgot to change its name.




And the name is misleading, it doesn't compute a power of two, it
computes the base-2 logarithm (then adds one), so it's a compile-time
version of floor(log2(n))+1.


@@ -123,6 +125,36 @@ namespace __gnu_debug
  template
void
_M_transfer_from_if(_Safe_sequence& __from, _Predicate __pred);
+
+  static _GLIBCXX_CONSTEXPR std::size_t
+  _S_alignment() _GLIBCXX_NOEXCEPT
+  { return _Lowest_power_of_two<__alignof(_Sequence)>::__val; }


This function is called "alignment" but it returns log2(alignof(T))+1


Yes, I wasn't proud about the naming.



Does it need to be constexpr? Is that for std::array?


No, it is not used in std::array context. I just considered that it 
could be constexpr.




You can get the same result without _Lowest_power_of_two:

 static _GLIBCXX_CONSTEXPR size_t
 _S_alignbits() _GLIBCXX_NOEXCEPT
 { return __builtin_ctz(__alignof(_Sequence)) + 1; }

But I'm not convinced the +1 is correct, see below.


@@ -46,15 +47,30 @@ namespace
  /** Returns different instances of __mutex depending on the passed 
address
   *  in order to limit contention without breaking current library 
binary

   *  compatibility. */
+  const size_t mask = 0xf;
+
  __gnu_cxx::__mutex&
-  get_safe_base_mutex(void* __address)
+  get_mutex(size_t index)
  {
-const size_t mask = 0xf;
static __gnu_cxx::__mutex safe_base_mutex[mask + 1];
-const size_t index = _Hash_impl::hash(__address) & mask;
return safe_base_mutex[index];
  }


N.B. https://gcc.gnu.org/PR71312
Ok, debug mode is also impacted. Shouldn't the alignment be set on the 
__mutex type directly ?





+  __gnu_cxx::__mutex&
+  get_safe_base_mutex(void* address)
+  {
+const size_t index = _Hash_impl::hash(address) & mask;
+return get_mutex(index);
+  }
+
+  __gnu_cxx::__mutex&
+  

Re: debug container mutex association

[Jonathan Wakely]

On 13/09/16 22:37 +0200, François Dumont wrote:

Hi

   When I proposed to change std::hash for pointers my plan was to 
use this approach for the debug containers. So here is the patch 
leveraging on this technique to avoid going through _Hash_impl to hash 
address and get mutex index from it. I think it is obious that new 
access is faster so I didn't wrote a performance test to demonstrate 
it.


Is this actually a bottleneck that needs to be made faster?

I know it's nice if Debug Mode isn't very slow, but you've already
made it much better, and performance is not the primary goal of Debug
Mode.


diff --git a/libstdc++-v3/config/abi/pre/gnu.ver 
b/libstdc++-v3/config/abi/pre/gnu.ver
index 9b5bb23..c9a253e 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -1947,6 +1947,21 @@ GLIBCXX_3.4.23 {
_ZNSsC[12]ERKSs[jmy]RKSaIcE;
_ZNSbIwSt11char_traitsIwESaIwEEC[12]ERKS2_mRKS1_;

+# __gnu_debug::_Safe_sequence_base
+_ZN11__gnu_debug19_Safe_sequence_base18_M_detach_singularEm;


The 'm' here should be [jmy] because std::size_t mangles differently
on different targets.


+_ZN11__gnu_debug19_Safe_sequence_base22_M_revalidate_singularEm;
+_ZN11__gnu_debug19_Safe_sequence_base12_M_get_mutexEm;
+_ZN11__gnu_debug19_Safe_sequence_base7_M_swapERS0_m;
+
+# __gnu_debug::_Safe_iterator_base
+
_ZN11__gnu_debug19_Safe_iterator_base9_M_attachEPNS_19_Safe_sequence_baseEbm;
+_ZN11__gnu_debug19_Safe_iterator_base9_M_detachEm;
+
+# __gnu_debug::_Safe_unordered_container_base and _Safe_local_iterator_base
+_ZN11__gnu_debug30_Safe_unordered_container_base7_M_swapERS0_m;
+
_ZN11__gnu_debug25_Safe_local_iterator_base9_M_attachEPNS_19_Safe_sequence_baseEbm;
+_ZN11__gnu_debug25_Safe_local_iterator_base9_M_detachEm;
+
} GLIBCXX_3.4.22;


It's unfortunate to have to export and maintain new symbols when we
don't offer the same level of ABI guarantee for Debug Mode  :-(


@@ -174,6 +191,18 @@ namespace __gnu_debug
const _Safe_iterator_base* __last)
  { return __first->_M_can_compare(*__last); }

+  // Compute power of 2 not greater than __n.
+  template
+struct _Lowest_power_of_two
+{
+  static const size_t __val
+= _Lowest_power_of_two< (__n >> 1) >::__val + 1;
+};
+
+  template<>
+struct _Lowest_power_of_two<1>
+{ static const size_t __val = 1; };


The lowest power of two not greater than 1 is 2^0, but this gives the
result 1.

Similarly, the lowest power of two not greater than 2 is 2^1, i.e. 1.

And the name is misleading, it doesn't compute a power of two, it
computes the base-2 logarithm (then adds one), so it's a compile-time
version of floor(log2(n))+1.


@@ -123,6 +125,36 @@ namespace __gnu_debug
  template
void
_M_transfer_from_if(_Safe_sequence& __from, _Predicate __pred);
+
+  static _GLIBCXX_CONSTEXPR std::size_t
+  _S_alignment() _GLIBCXX_NOEXCEPT
+  { return _Lowest_power_of_two<__alignof(_Sequence)>::__val; }


This function is called "alignment" but it returns log2(alignof(T))+1

Does it need to be constexpr? Is that for std::array?

You can get the same result without _Lowest_power_of_two:

 static _GLIBCXX_CONSTEXPR size_t
 _S_alignbits() _GLIBCXX_NOEXCEPT
 { return __builtin_ctz(__alignof(_Sequence)) + 1; }

But I'm not convinced the +1 is correct, see below.


@@ -46,15 +47,30 @@ namespace
  /** Returns different instances of __mutex depending on the passed address
   *  in order to limit contention without breaking current library binary
   *  compatibility. */
+  const size_t mask = 0xf;
+
  __gnu_cxx::__mutex&
-  get_safe_base_mutex(void* __address)
+  get_mutex(size_t index)
  {
-const size_t mask = 0xf;
static __gnu_cxx::__mutex safe_base_mutex[mask + 1];
-const size_t index = _Hash_impl::hash(__address) & mask;
return safe_base_mutex[index];
  }


N.B. https://gcc.gnu.org/PR71312



+  __gnu_cxx::__mutex&
+  get_safe_base_mutex(void* address)
+  {
+const size_t index = _Hash_impl::hash(address) & mask;
+return get_mutex(index);
+  }
+
+  __gnu_cxx::__mutex&
+  get_safe_base_mutex(void* address, std::size_t alignment)
+  {
+const size_t index
+  = (reinterpret_cast(address) >> alignment) & mask;
+return get_mutex(index);
+  }


Since the "alignment" value (which is really log2(align)+1) is always
a positive value that means we always shift the address. But that
means we drop the least significant bit of all pointers, even when the
type has alignof(T)==1 and so all bits in the pointer are significant.
Is that intentional? Is the idea to only drop bits that are always
zero?

By changing the algorithm we use to map an address to a mutex do we
allow programs to get two different mutexes for the same object? That
can only happen when some objects are compiled with an older GCC and
so still use the old _M_get_mutex() function that doesn't take an
argument. We require all objects to be rebuilt with the