Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]

Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]

[Jonathan Wakely]

On 09/08/23 01:34 +0300, Vladimir Palevich wrote:
>Because of the recent change in _M_realloc_insert and _M_default_append, call
>to deallocate was ordered after assignment to class members of std::vector
>(in the guard destructor), which is causing said members to be call-clobbered.
>This is preventing further optimization, the compiler is unable to move memory
>read out of a hot loop in this case.
>This patch reorders the call to before assignments by putting guard in its own
>block. Plus a new testsuite for this case.
>I'm not very happy with the new testsuite, but I don't know how to properly
>test this.

Thanks for the patch, and for figuring out what caused the regression.

>Tested on x86_64-pc-linux-gnu.
>
>Maybe something could be done so that the compiler would be able to optimize
>such cases anyway. Reads could be moved just after the clobbering calls in
>unlikely branches, for example. This should be a fairly common case with
>destructors at the end of a function.
>
>Note: I don't have write access.

OK, thanks, I'll take care of it.

N.B. libstdc++ patches should also be CC'd to the libstdc++ list,
otherwise I won't see them.

>-- >8 --
>
>Fix ordering to prevent clobbering of class members by a call to deallocate
>in _M_realloc_insert and _M_default_append.
>
>libstdc++-v3/ChangeLog:
>    PR libstdc++/110879
>    * include/bits/vector.tcc: End guard lifetime just before assignment to
>    class members.
>    * testsuite/libstdc++-dg/conformance.exp: Load scantree.exp.
>    * testsuite/23_containers/vector/110879.cc: New test.
>
>Signed-off-by: Vladimir Palevich  <palevichva@gmail.com>
>---
> libstdc++-v3/include/bits/vector.tcc          | 220 +++++++++---------
> .../testsuite/23_containers/vector/110879.cc  |  35 +++
> .../testsuite/libstdc++-dg/conformance.exp    |  13 ++
> 3 files changed, 163 insertions(+), 105 deletions(-)
> create mode 100644 libstdc++-v3/testsuite/23_containers/vector/110879.cc
>
>diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
>index ada396c9b30..80631d1e2a1 100644
>--- a/libstdc++-v3/include/bits/vector.tcc
>+++ b/libstdc++-v3/include/bits/vector.tcc
>@@ -488,78 +488,83 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>       private:
> 	_Guard(const _Guard&);
>       };
>-      _Guard __guard(__new_start, __len, _M_impl);
>
>-      // The order of the three operations is dictated by the C++11
>-      // case, where the moves could alter a new element belonging
>-      // to the existing vector.  This is an issue only for callers
>-      // taking the element by lvalue ref (see last bullet of C++11
>-      // [res.on.arguments]).
>+      {
>+	_Guard __guard(__new_start, __len, _M_impl);
>
>-      // If this throws, the existing elements are unchanged.
>+	// The order of the three operations is dictated by the C++11
>+	// case, where the moves could alter a new element belonging
>+	// to the existing vector.  This is an issue only for callers
>+	// taking the element by lvalue ref (see last bullet of C++11
>+	// [res.on.arguments]).
>+
>+	// If this throws, the existing elements are unchanged.
> #if __cplusplus >= 201103L
>-      _Alloc_traits::construct(this->_M_impl,
>-			       std::__to_address(__new_start + __elems_before),
>-			       std::forward<_Args>(__args)...);
>+	_Alloc_traits::construct(this->_M_impl,
>+				 std::__to_address(__new_start + __elems_before),
>+				 std::forward<_Args>(__args)...);
> #else
>-      _Alloc_traits::construct(this->_M_impl,
>-			       __new_start + __elems_before,
>-			       __x);
>+	_Alloc_traits::construct(this->_M_impl,
>+				 __new_start + __elems_before,
>+				 __x);
> #endif
>
> #if __cplusplus >= 201103L
>-      if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
>-	{
>-	  // Relocation cannot throw.
>-	  __new_finish = _S_relocate(__old_start, __position.base(),
>-				     __new_start, _M_get_Tp_allocator());
>-	  ++__new_finish;
>-	  __new_finish = _S_relocate(__position.base(), __old_finish,
>-				     __new_finish, _M_get_Tp_allocator());
>-	}
>-      else
>+	if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
>+	  {
>+	    // Relocation cannot throw.
>+	    __new_finish = _S_relocate(__old_start, __position.base(),
>+				       __new_start, _M_get_Tp_allocator());
>+	    ++__new_finish;
>+	    __new_finish = _S_relocate(__position.base(), __old_finish,
>+				       __new_finish, _M_get_Tp_allocator());
>+	  }
>+	else
> #endif
>-	{
>-	  // RAII type to destroy initialized elements.
>-	  struct _Guard_elts
> 	  {
>-	    pointer _M_first, _M_last;  // Elements to destroy
>-	    _Tp_alloc_type& _M_alloc;
>-
>-	    _GLIBCXX20_CONSTEXPR
>-	    _Guard_elts(pointer __elt, _Tp_alloc_type& __a)
>-	    : _M_first(__elt), _M_last(__elt + 1), _M_alloc(__a)
>-	    { }
>-
>-	    _GLIBCXX20_CONSTEXPR
>-	    ~_Guard_elts()
>-	    { std::_Destroy(_M_first, _M_last, _M_alloc); }
>-
>-	  private:
>-	    _Guard_elts(const _Guard_elts&);
>-	  };
>-
>-	  // Guard the new element so it will be destroyed if anything throws.
>-	  _Guard_elts __guard_elts(__new_start + __elems_before, _M_impl);
>-
>-	  __new_finish = std::__uninitialized_move_if_noexcept_a(
>-			   __old_start, __position.base(),
>-			   __new_start, _M_get_Tp_allocator());
>-
>-	  ++__new_finish;
>-	  // Guard everything before the new element too.
>-	  __guard_elts._M_first = __new_start;
>-
>-	  __new_finish = std::__uninitialized_move_if_noexcept_a(
>-			    __position.base(), __old_finish,
>-			    __new_finish, _M_get_Tp_allocator());
>-
>-	  // New storage has been fully initialized, destroy the old elements.
>-	  __guard_elts._M_first = __old_start;
>-	  __guard_elts._M_last = __old_finish;
>-	}
>-      __guard._M_storage = __old_start;
>-      __guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
>+	    // RAII type to destroy initialized elements.
>+	    struct _Guard_elts
>+	    {
>+	      pointer _M_first, _M_last;  // Elements to destroy
>+	      _Tp_alloc_type& _M_alloc;
>+
>+	      _GLIBCXX20_CONSTEXPR
>+	      _Guard_elts(pointer __elt, _Tp_alloc_type& __a)
>+	      : _M_first(__elt), _M_last(__elt + 1), _M_alloc(__a)
>+	      { }
>+
>+	      _GLIBCXX20_CONSTEXPR
>+	      ~_Guard_elts()
>+	      { std::_Destroy(_M_first, _M_last, _M_alloc); }
>+
>+	    private:
>+	      _Guard_elts(const _Guard_elts&);
>+	    };
>+
>+	    // Guard the new element so it will be destroyed if anything throws.
>+	    _Guard_elts __guard_elts(__new_start + __elems_before, _M_impl);
>+
>+	    __new_finish = std::__uninitialized_move_if_noexcept_a(
>+			     __old_start, __position.base(),
>+			     __new_start, _M_get_Tp_allocator());
>+
>+	    ++__new_finish;
>+	    // Guard everything before the new element too.
>+	    __guard_elts._M_first = __new_start;
>+
>+	    __new_finish = std::__uninitialized_move_if_noexcept_a(
>+			      __position.base(), __old_finish,
>+			      __new_finish, _M_get_Tp_allocator());
>+
>+	    // New storage has been fully initialized, destroy the old elements.
>+	    __guard_elts._M_first = __old_start;
>+	    __guard_elts._M_last = __old_finish;
>+	  }
>+	__guard._M_storage = __old_start;
>+	__guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
>+      }
>+      // deallocate should be called before assignments to _M_impl,
>+      // to avoid call-clobbering
>
>       this->_M_impl._M_start = __new_start;
>       this->_M_impl._M_finish = __new_finish;
>@@ -728,49 +733,54 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> 	      private:
> 		_Guard(const _Guard&);
> 	      };
>-	      _Guard __guard(__new_start, __len, _M_impl);
>-
>-	      std::__uninitialized_default_n_a(__new_start + __size, __n,
>-					       _M_get_Tp_allocator());
>-
>-	      if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
>-		{
>-		  _S_relocate(__old_start, __old_finish,
>-			      __new_start, _M_get_Tp_allocator());
>-		}
>-	      else
>-		{
>-		  // RAII type to destroy initialized elements.
>-		  struct _Guard_elts
>+
>+	      {
>+		_Guard __guard(__new_start, __len, _M_impl);
>+
>+		std::__uninitialized_default_n_a(__new_start + __size, __n,
>+						 _M_get_Tp_allocator());
>+
>+		if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
> 		  {
>-		    pointer _M_first, _M_last;  // Elements to destroy
>-		    _Tp_alloc_type& _M_alloc;
>-
>-		    _GLIBCXX20_CONSTEXPR
>-		    _Guard_elts(pointer __first, size_type __n,
>-				_Tp_alloc_type& __a)
>-		    : _M_first(__first), _M_last(__first + __n), _M_alloc(__a)
>-		    { }
>-
>-		    _GLIBCXX20_CONSTEXPR
>-		    ~_Guard_elts()
>-		    { std::_Destroy(_M_first, _M_last, _M_alloc); }
>-
>-		  private:
>-		    _Guard_elts(const _Guard_elts&);
>-		  };
>-		  _Guard_elts __guard_elts(__new_start + __size, __n, _M_impl);
>-
>-		  std::__uninitialized_move_if_noexcept_a(
>-		    __old_start, __old_finish, __new_start,
>-		    _M_get_Tp_allocator());
>-
>-		  __guard_elts._M_first = __old_start;
>-		  __guard_elts._M_last = __old_finish;
>-		}
>-	      _GLIBCXX_ASAN_ANNOTATE_REINIT;
>-	      __guard._M_storage = __old_start;
>-	      __guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
>+		    _S_relocate(__old_start, __old_finish,
>+				__new_start, _M_get_Tp_allocator());
>+		  }
>+		else
>+		  {
>+		    // RAII type to destroy initialized elements.
>+		    struct _Guard_elts
>+		    {
>+		      pointer _M_first, _M_last;  // Elements to destroy
>+		      _Tp_alloc_type& _M_alloc;
>+
>+		      _GLIBCXX20_CONSTEXPR
>+		      _Guard_elts(pointer __first, size_type __n,
>+				  _Tp_alloc_type& __a)
>+		      : _M_first(__first), _M_last(__first + __n), _M_alloc(__a)
>+		      { }
>+
>+		      _GLIBCXX20_CONSTEXPR
>+		      ~_Guard_elts()
>+		      { std::_Destroy(_M_first, _M_last, _M_alloc); }
>+
>+		    private:
>+		      _Guard_elts(const _Guard_elts&);
>+		    };
>+		    _Guard_elts __guard_elts(__new_start + __size, __n, _M_impl);
>+
>+		    std::__uninitialized_move_if_noexcept_a(
>+		      __old_start, __old_finish, __new_start,
>+		      _M_get_Tp_allocator());
>+
>+		    __guard_elts._M_first = __old_start;
>+		    __guard_elts._M_last = __old_finish;
>+		  }
>+		_GLIBCXX_ASAN_ANNOTATE_REINIT;
>+		__guard._M_storage = __old_start;
>+		__guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
>+	      }
>+	      // deallocate should be called before assignments to _M_impl,
>+	      // to avoid call-clobbering
>
> 	      this->_M_impl._M_start = __new_start;
> 	      this->_M_impl._M_finish = __new_start + __size + __n;
>diff --git a/libstdc++-v3/testsuite/23_containers/vector/110879.cc b/libstdc++-v3/testsuite/23_containers/vector/110879.cc
>new file mode 100644
>index 00000000000..d38a5a0d1a3
>--- /dev/null
>+++ b/libstdc++-v3/testsuite/23_containers/vector/110879.cc
>@@ -0,0 +1,35 @@
>+// -*- C++ -*-
>+
>+// Copyright (C) 2023 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/>.
>+
>+// { dg-do compile }
>+// { dg-options "-O3 -fdump-tree-optimized" }
>+
>+#include <vector>
>+
>+std::vector<int> f(std::size_t n) {
>+  std::vector<int> res;
>+  for (std::size_t i = 0; i < n; ++i) {
>+    res.push_back(i);
>+  }
>+  return res;
>+}
>+
>+// Reads of _M_finish should be optimized out.
>+// This regex matches all reads from res variable except for _M_end_of_storage field.
>+// { dg-final { scan-tree-dump-not "=\\s*\\S*res_(?!\\S*_M_end_of_storage;)" "optimized" } }
>diff --git a/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp b/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
>index e8c6504a7f7..1d0588aeadc 100644
>--- a/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
>+++ b/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
>@@ -18,6 +18,19 @@
>
> # libstdc++-v3 testsuite that uses the 'dg.exp' driver.
>
>+# Damn dejagnu for not having proper library search paths for load_lib.
>+# We have to explicitly load everything that gcc-dg.exp wants to load.
>+
>+proc load_gcc_lib { filename } {
>+    global srcdir loaded_libs
>+
>+    load_file $srcdir/../../gcc/testsuite/lib/$filename
>+    set loaded_libs($filename) ""
>+}
>+
>+load_gcc_lib scandump.exp
>+load_gcc_lib scantree.exp
>+
> # Initialization.
> dg-init
>

Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]

[Jonathan Wakely]

On 09/08/23 01:34 +0300, Vladimir Palevich wrote:
>Because of the recent change in _M_realloc_insert and _M_default_append, call
>to deallocate was ordered after assignment to class members of std::vector
>(in the guard destructor), which is causing said members to be call-clobbered.
>This is preventing further optimization, the compiler is unable to move memory
>read out of a hot loop in this case.
>This patch reorders the call to before assignments by putting guard in its own
>block. Plus a new testsuite for this case.
>I'm not very happy with the new testsuite, but I don't know how to properly
>test this.
>
>Tested on x86_64-pc-linux-gnu.
>
>Maybe something could be done so that the compiler would be able to optimize
>such cases anyway. Reads could be moved just after the clobbering calls in
>unlikely branches, for example. This should be a fairly common case with
>destructors at the end of a function.
>
>Note: I don't have write access.
>
>-- >8 --
>
>Fix ordering to prevent clobbering of class members by a call to deallocate
>in _M_realloc_insert and _M_default_append.
>
>libstdc++-v3/ChangeLog:
>    PR libstdc++/110879
>    * include/bits/vector.tcc: End guard lifetime just before assignment to
>    class members.
>    * testsuite/libstdc++-dg/conformance.exp: Load scantree.exp.
>    * testsuite/23_containers/vector/110879.cc: New test.
>
>Signed-off-by: Vladimir Palevich  <palevichva@gmail.com>
>---
> libstdc++-v3/include/bits/vector.tcc          | 220 +++++++++---------
> .../testsuite/23_containers/vector/110879.cc  |  35 +++
> .../testsuite/libstdc++-dg/conformance.exp    |  13 ++
> 3 files changed, 163 insertions(+), 105 deletions(-)
> create mode 100644 libstdc++-v3/testsuite/23_containers/vector/110879.cc
>
>diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
>index ada396c9b30..80631d1e2a1 100644
>--- a/libstdc++-v3/include/bits/vector.tcc
>+++ b/libstdc++-v3/include/bits/vector.tcc
>@@ -488,78 +488,83 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>       private:
> 	_Guard(const _Guard&);
>       };
>-      _Guard __guard(__new_start, __len, _M_impl);
>
>-      // The order of the three operations is dictated by the C++11
>-      // case, where the moves could alter a new element belonging
>-      // to the existing vector.  This is an issue only for callers
>-      // taking the element by lvalue ref (see last bullet of C++11
>-      // [res.on.arguments]).
>+      {
>+	_Guard __guard(__new_start, __len, _M_impl);
>
>-      // If this throws, the existing elements are unchanged.
>+	// The order of the three operations is dictated by the C++11
>+	// case, where the moves could alter a new element belonging
>+	// to the existing vector.  This is an issue only for callers
>+	// taking the element by lvalue ref (see last bullet of C++11
>+	// [res.on.arguments]).
>+
>+	// If this throws, the existing elements are unchanged.
> #if __cplusplus >= 201103L
>-      _Alloc_traits::construct(this->_M_impl,
>-			       std::__to_address(__new_start + __elems_before),
>-			       std::forward<_Args>(__args)...);
>+	_Alloc_traits::construct(this->_M_impl,
>+				 std::__to_address(__new_start + __elems_before),
>+				 std::forward<_Args>(__args)...);
> #else
>-      _Alloc_traits::construct(this->_M_impl,
>-			       __new_start + __elems_before,
>-			       __x);
>+	_Alloc_traits::construct(this->_M_impl,
>+				 __new_start + __elems_before,
>+				 __x);
> #endif
>
> #if __cplusplus >= 201103L
>-      if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
>-	{
>-	  // Relocation cannot throw.
>-	  __new_finish = _S_relocate(__old_start, __position.base(),
>-				     __new_start, _M_get_Tp_allocator());
>-	  ++__new_finish;
>-	  __new_finish = _S_relocate(__position.base(), __old_finish,
>-				     __new_finish, _M_get_Tp_allocator());
>-	}
>-      else
>+	if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
>+	  {
>+	    // Relocation cannot throw.
>+	    __new_finish = _S_relocate(__old_start, __position.base(),
>+				       __new_start, _M_get_Tp_allocator());
>+	    ++__new_finish;
>+	    __new_finish = _S_relocate(__position.base(), __old_finish,
>+				       __new_finish, _M_get_Tp_allocator());
>+	  }
>+	else
> #endif
>-	{
>-	  // RAII type to destroy initialized elements.
>-	  struct _Guard_elts
> 	  {
>-	    pointer _M_first, _M_last;  // Elements to destroy
>-	    _Tp_alloc_type& _M_alloc;
>-
>-	    _GLIBCXX20_CONSTEXPR
>-	    _Guard_elts(pointer __elt, _Tp_alloc_type& __a)
>-	    : _M_first(__elt), _M_last(__elt + 1), _M_alloc(__a)
>-	    { }
>-
>-	    _GLIBCXX20_CONSTEXPR
>-	    ~_Guard_elts()
>-	    { std::_Destroy(_M_first, _M_last, _M_alloc); }
>-
>-	  private:
>-	    _Guard_elts(const _Guard_elts&);
>-	  };
>-
>-	  // Guard the new element so it will be destroyed if anything throws.
>-	  _Guard_elts __guard_elts(__new_start + __elems_before, _M_impl);
>-
>-	  __new_finish = std::__uninitialized_move_if_noexcept_a(
>-			   __old_start, __position.base(),
>-			   __new_start, _M_get_Tp_allocator());
>-
>-	  ++__new_finish;
>-	  // Guard everything before the new element too.
>-	  __guard_elts._M_first = __new_start;
>-
>-	  __new_finish = std::__uninitialized_move_if_noexcept_a(
>-			    __position.base(), __old_finish,
>-			    __new_finish, _M_get_Tp_allocator());
>-
>-	  // New storage has been fully initialized, destroy the old elements.
>-	  __guard_elts._M_first = __old_start;
>-	  __guard_elts._M_last = __old_finish;
>-	}
>-      __guard._M_storage = __old_start;
>-      __guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
>+	    // RAII type to destroy initialized elements.
>+	    struct _Guard_elts
>+	    {
>+	      pointer _M_first, _M_last;  // Elements to destroy
>+	      _Tp_alloc_type& _M_alloc;
>+
>+	      _GLIBCXX20_CONSTEXPR
>+	      _Guard_elts(pointer __elt, _Tp_alloc_type& __a)
>+	      : _M_first(__elt), _M_last(__elt + 1), _M_alloc(__a)
>+	      { }
>+
>+	      _GLIBCXX20_CONSTEXPR
>+	      ~_Guard_elts()
>+	      { std::_Destroy(_M_first, _M_last, _M_alloc); }
>+
>+	    private:
>+	      _Guard_elts(const _Guard_elts&);
>+	    };
>+
>+	    // Guard the new element so it will be destroyed if anything throws.
>+	    _Guard_elts __guard_elts(__new_start + __elems_before, _M_impl);
>+
>+	    __new_finish = std::__uninitialized_move_if_noexcept_a(
>+			     __old_start, __position.base(),
>+			     __new_start, _M_get_Tp_allocator());
>+
>+	    ++__new_finish;
>+	    // Guard everything before the new element too.
>+	    __guard_elts._M_first = __new_start;
>+
>+	    __new_finish = std::__uninitialized_move_if_noexcept_a(
>+			      __position.base(), __old_finish,
>+			      __new_finish, _M_get_Tp_allocator());
>+
>+	    // New storage has been fully initialized, destroy the old elements.
>+	    __guard_elts._M_first = __old_start;
>+	    __guard_elts._M_last = __old_finish;
>+	  }
>+	__guard._M_storage = __old_start;
>+	__guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
>+      }
>+      // deallocate should be called before assignments to _M_impl,
>+      // to avoid call-clobbering
>
>       this->_M_impl._M_start = __new_start;
>       this->_M_impl._M_finish = __new_finish;
>@@ -728,49 +733,54 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> 	      private:
> 		_Guard(const _Guard&);
> 	      };
>-	      _Guard __guard(__new_start, __len, _M_impl);
>-
>-	      std::__uninitialized_default_n_a(__new_start + __size, __n,
>-					       _M_get_Tp_allocator());
>-
>-	      if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
>-		{
>-		  _S_relocate(__old_start, __old_finish,
>-			      __new_start, _M_get_Tp_allocator());
>-		}
>-	      else
>-		{
>-		  // RAII type to destroy initialized elements.
>-		  struct _Guard_elts
>+
>+	      {
>+		_Guard __guard(__new_start, __len, _M_impl);
>+
>+		std::__uninitialized_default_n_a(__new_start + __size, __n,
>+						 _M_get_Tp_allocator());
>+
>+		if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
> 		  {
>-		    pointer _M_first, _M_last;  // Elements to destroy
>-		    _Tp_alloc_type& _M_alloc;
>-
>-		    _GLIBCXX20_CONSTEXPR
>-		    _Guard_elts(pointer __first, size_type __n,
>-				_Tp_alloc_type& __a)
>-		    : _M_first(__first), _M_last(__first + __n), _M_alloc(__a)
>-		    { }
>-
>-		    _GLIBCXX20_CONSTEXPR
>-		    ~_Guard_elts()
>-		    { std::_Destroy(_M_first, _M_last, _M_alloc); }
>-
>-		  private:
>-		    _Guard_elts(const _Guard_elts&);
>-		  };
>-		  _Guard_elts __guard_elts(__new_start + __size, __n, _M_impl);
>-
>-		  std::__uninitialized_move_if_noexcept_a(
>-		    __old_start, __old_finish, __new_start,
>-		    _M_get_Tp_allocator());
>-
>-		  __guard_elts._M_first = __old_start;
>-		  __guard_elts._M_last = __old_finish;
>-		}
>-	      _GLIBCXX_ASAN_ANNOTATE_REINIT;
>-	      __guard._M_storage = __old_start;
>-	      __guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
>+		    _S_relocate(__old_start, __old_finish,
>+				__new_start, _M_get_Tp_allocator());
>+		  }
>+		else
>+		  {
>+		    // RAII type to destroy initialized elements.
>+		    struct _Guard_elts
>+		    {
>+		      pointer _M_first, _M_last;  // Elements to destroy
>+		      _Tp_alloc_type& _M_alloc;
>+
>+		      _GLIBCXX20_CONSTEXPR
>+		      _Guard_elts(pointer __first, size_type __n,
>+				  _Tp_alloc_type& __a)
>+		      : _M_first(__first), _M_last(__first + __n), _M_alloc(__a)
>+		      { }
>+
>+		      _GLIBCXX20_CONSTEXPR
>+		      ~_Guard_elts()
>+		      { std::_Destroy(_M_first, _M_last, _M_alloc); }
>+
>+		    private:
>+		      _Guard_elts(const _Guard_elts&);
>+		    };
>+		    _Guard_elts __guard_elts(__new_start + __size, __n, _M_impl);
>+
>+		    std::__uninitialized_move_if_noexcept_a(
>+		      __old_start, __old_finish, __new_start,
>+		      _M_get_Tp_allocator());
>+
>+		    __guard_elts._M_first = __old_start;
>+		    __guard_elts._M_last = __old_finish;
>+		  }
>+		_GLIBCXX_ASAN_ANNOTATE_REINIT;
>+		__guard._M_storage = __old_start;
>+		__guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
>+	      }
>+	      // deallocate should be called before assignments to _M_impl,
>+	      // to avoid call-clobbering
>
> 	      this->_M_impl._M_start = __new_start;
> 	      this->_M_impl._M_finish = __new_start + __size + __n;
>diff --git a/libstdc++-v3/testsuite/23_containers/vector/110879.cc b/libstdc++-v3/testsuite/23_containers/vector/110879.cc
>new file mode 100644
>index 00000000000..d38a5a0d1a3
>--- /dev/null
>+++ b/libstdc++-v3/testsuite/23_containers/vector/110879.cc
>@@ -0,0 +1,35 @@
>+// -*- C++ -*-
>+
>+// Copyright (C) 2023 Free Software Foundation, Inc.

Do you actually have a FSF copyright assignment for GCC?

If not, then this should not be here, as you can't assign it to the
FSF.

You've already added a Signed-off-by tag, which I assume means you're
contributing under the terms of the DCO: https://gcc.gnu.org/dco.html

In which case, this isn't copyright FSF.

I've stopped putting the copyright and license header in tests, I
don't consider a 5 line function that does nothing interesting to be
copyrightable, or worth adding all this boilerplate to.

If you don't mind, I'll just remove this boilerplate from the test.


>+// 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/>.
>+
>+// { dg-do compile }
>+// { dg-options "-O3 -fdump-tree-optimized" }
>+
>+#include <vector>
>+
>+std::vector<int> f(std::size_t n) {
>+  std::vector<int> res;
>+  for (std::size_t i = 0; i < n; ++i) {
>+    res.push_back(i);
>+  }
>+  return res;
>+}
>+
>+// Reads of _M_finish should be optimized out.
>+// This regex matches all reads from res variable except for _M_end_of_storage field.
>+// { dg-final { scan-tree-dump-not "=\\s*\\S*res_(?!\\S*_M_end_of_storage;)" "optimized" } }

If this was added to gcc/testsuite/g++.dg/ instead of
libstdc++-v3/testsuite then we wouldn't need scandump.exp and
scantree.exp in the library tests.

>diff --git a/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp b/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
>index e8c6504a7f7..1d0588aeadc 100644
>--- a/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
>+++ b/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
>@@ -18,6 +18,19 @@
>
> # libstdc++-v3 testsuite that uses the 'dg.exp' driver.
>
>+# Damn dejagnu for not having proper library search paths for load_lib.
>+# We have to explicitly load everything that gcc-dg.exp wants to load.
>+
>+proc load_gcc_lib { filename } {
>+    global srcdir loaded_libs
>+
>+    load_file $srcdir/../../gcc/testsuite/lib/$filename
>+    set loaded_libs($filename) ""
>+}
>+
>+load_gcc_lib scandump.exp
>+load_gcc_lib scantree.exp
>+
> # Initialization.
> dg-init
>

Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]

[Vladimir Palevich]

On Thu, 17 Aug 2023 at 01:51, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On 09/08/23 01:34 +0300, Vladimir Palevich wrote:
> >Because of the recent change in _M_realloc_insert and _M_default_append, call
> >to deallocate was ordered after assignment to class members of std::vector
> >(in the guard destructor), which is causing said members to be call-clobbered.
> >This is preventing further optimization, the compiler is unable to move memory
> >read out of a hot loop in this case.
> >This patch reorders the call to before assignments by putting guard in its own
> >block. Plus a new testsuite for this case.
> >I'm not very happy with the new testsuite, but I don't know how to properly
> >test this.
> >
> >Tested on x86_64-pc-linux-gnu.
> >
> >Maybe something could be done so that the compiler would be able to optimize
> >such cases anyway. Reads could be moved just after the clobbering calls in
> >unlikely branches, for example. This should be a fairly common case with
> >destructors at the end of a function.
> >
> >Note: I don't have write access.
> >
> >-- >8 --
> >
> >Fix ordering to prevent clobbering of class members by a call to deallocate
> >in _M_realloc_insert and _M_default_append.
> >
> >libstdc++-v3/ChangeLog:
> >    PR libstdc++/110879
> >    * include/bits/vector.tcc: End guard lifetime just before assignment to
> >    class members.
> >    * testsuite/libstdc++-dg/conformance.exp: Load scantree.exp.
> >    * testsuite/23_containers/vector/110879.cc: New test.
> >
> >Signed-off-by: Vladimir Palevich  <palevichva@gmail.com>
> >---
> > libstdc++-v3/include/bits/vector.tcc          | 220 +++++++++---------
> > .../testsuite/23_containers/vector/110879.cc  |  35 +++
> > .../testsuite/libstdc++-dg/conformance.exp    |  13 ++
> > 3 files changed, 163 insertions(+), 105 deletions(-)
> > create mode 100644 libstdc++-v3/testsuite/23_containers/vector/110879.cc
> >
> >diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
> >index ada396c9b30..80631d1e2a1 100644
> >--- a/libstdc++-v3/include/bits/vector.tcc
> >+++ b/libstdc++-v3/include/bits/vector.tcc
> >@@ -488,78 +488,83 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> >       private:
> >       _Guard(const _Guard&);
> >       };
> >-      _Guard __guard(__new_start, __len, _M_impl);
> >
> >-      // The order of the three operations is dictated by the C++11
> >-      // case, where the moves could alter a new element belonging
> >-      // to the existing vector.  This is an issue only for callers
> >-      // taking the element by lvalue ref (see last bullet of C++11
> >-      // [res.on.arguments]).
> >+      {
> >+      _Guard __guard(__new_start, __len, _M_impl);
> >
> >-      // If this throws, the existing elements are unchanged.
> >+      // The order of the three operations is dictated by the C++11
> >+      // case, where the moves could alter a new element belonging
> >+      // to the existing vector.  This is an issue only for callers
> >+      // taking the element by lvalue ref (see last bullet of C++11
> >+      // [res.on.arguments]).
> >+
> >+      // If this throws, the existing elements are unchanged.
> > #if __cplusplus >= 201103L
> >-      _Alloc_traits::construct(this->_M_impl,
> >-                             std::__to_address(__new_start + __elems_before),
> >-                             std::forward<_Args>(__args)...);
> >+      _Alloc_traits::construct(this->_M_impl,
> >+                               std::__to_address(__new_start + __elems_before),
> >+                               std::forward<_Args>(__args)...);
> > #else
> >-      _Alloc_traits::construct(this->_M_impl,
> >-                             __new_start + __elems_before,
> >-                             __x);
> >+      _Alloc_traits::construct(this->_M_impl,
> >+                               __new_start + __elems_before,
> >+                               __x);
> > #endif
> >
> > #if __cplusplus >= 201103L
> >-      if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
> >-      {
> >-        // Relocation cannot throw.
> >-        __new_finish = _S_relocate(__old_start, __position.base(),
> >-                                   __new_start, _M_get_Tp_allocator());
> >-        ++__new_finish;
> >-        __new_finish = _S_relocate(__position.base(), __old_finish,
> >-                                   __new_finish, _M_get_Tp_allocator());
> >-      }
> >-      else
> >+      if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
> >+        {
> >+          // Relocation cannot throw.
> >+          __new_finish = _S_relocate(__old_start, __position.base(),
> >+                                     __new_start, _M_get_Tp_allocator());
> >+          ++__new_finish;
> >+          __new_finish = _S_relocate(__position.base(), __old_finish,
> >+                                     __new_finish, _M_get_Tp_allocator());
> >+        }
> >+      else
> > #endif
> >-      {
> >-        // RAII type to destroy initialized elements.
> >-        struct _Guard_elts
> >         {
> >-          pointer _M_first, _M_last;  // Elements to destroy
> >-          _Tp_alloc_type& _M_alloc;
> >-
> >-          _GLIBCXX20_CONSTEXPR
> >-          _Guard_elts(pointer __elt, _Tp_alloc_type& __a)
> >-          : _M_first(__elt), _M_last(__elt + 1), _M_alloc(__a)
> >-          { }
> >-
> >-          _GLIBCXX20_CONSTEXPR
> >-          ~_Guard_elts()
> >-          { std::_Destroy(_M_first, _M_last, _M_alloc); }
> >-
> >-        private:
> >-          _Guard_elts(const _Guard_elts&);
> >-        };
> >-
> >-        // Guard the new element so it will be destroyed if anything throws.
> >-        _Guard_elts __guard_elts(__new_start + __elems_before, _M_impl);
> >-
> >-        __new_finish = std::__uninitialized_move_if_noexcept_a(
> >-                         __old_start, __position.base(),
> >-                         __new_start, _M_get_Tp_allocator());
> >-
> >-        ++__new_finish;
> >-        // Guard everything before the new element too.
> >-        __guard_elts._M_first = __new_start;
> >-
> >-        __new_finish = std::__uninitialized_move_if_noexcept_a(
> >-                          __position.base(), __old_finish,
> >-                          __new_finish, _M_get_Tp_allocator());
> >-
> >-        // New storage has been fully initialized, destroy the old elements.
> >-        __guard_elts._M_first = __old_start;
> >-        __guard_elts._M_last = __old_finish;
> >-      }
> >-      __guard._M_storage = __old_start;
> >-      __guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
> >+          // RAII type to destroy initialized elements.
> >+          struct _Guard_elts
> >+          {
> >+            pointer _M_first, _M_last;  // Elements to destroy
> >+            _Tp_alloc_type& _M_alloc;
> >+
> >+            _GLIBCXX20_CONSTEXPR
> >+            _Guard_elts(pointer __elt, _Tp_alloc_type& __a)
> >+            : _M_first(__elt), _M_last(__elt + 1), _M_alloc(__a)
> >+            { }
> >+
> >+            _GLIBCXX20_CONSTEXPR
> >+            ~_Guard_elts()
> >+            { std::_Destroy(_M_first, _M_last, _M_alloc); }
> >+
> >+          private:
> >+            _Guard_elts(const _Guard_elts&);
> >+          };
> >+
> >+          // Guard the new element so it will be destroyed if anything throws.
> >+          _Guard_elts __guard_elts(__new_start + __elems_before, _M_impl);
> >+
> >+          __new_finish = std::__uninitialized_move_if_noexcept_a(
> >+                           __old_start, __position.base(),
> >+                           __new_start, _M_get_Tp_allocator());
> >+
> >+          ++__new_finish;
> >+          // Guard everything before the new element too.
> >+          __guard_elts._M_first = __new_start;
> >+
> >+          __new_finish = std::__uninitialized_move_if_noexcept_a(
> >+                            __position.base(), __old_finish,
> >+                            __new_finish, _M_get_Tp_allocator());
> >+
> >+          // New storage has been fully initialized, destroy the old elements.
> >+          __guard_elts._M_first = __old_start;
> >+          __guard_elts._M_last = __old_finish;
> >+        }
> >+      __guard._M_storage = __old_start;
> >+      __guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
> >+      }
> >+      // deallocate should be called before assignments to _M_impl,
> >+      // to avoid call-clobbering
> >
> >       this->_M_impl._M_start = __new_start;
> >       this->_M_impl._M_finish = __new_finish;
> >@@ -728,49 +733,54 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> >             private:
> >               _Guard(const _Guard&);
> >             };
> >-            _Guard __guard(__new_start, __len, _M_impl);
> >-
> >-            std::__uninitialized_default_n_a(__new_start + __size, __n,
> >-                                             _M_get_Tp_allocator());
> >-
> >-            if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
> >-              {
> >-                _S_relocate(__old_start, __old_finish,
> >-                            __new_start, _M_get_Tp_allocator());
> >-              }
> >-            else
> >-              {
> >-                // RAII type to destroy initialized elements.
> >-                struct _Guard_elts
> >+
> >+            {
> >+              _Guard __guard(__new_start, __len, _M_impl);
> >+
> >+              std::__uninitialized_default_n_a(__new_start + __size, __n,
> >+                                               _M_get_Tp_allocator());
> >+
> >+              if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
> >                 {
> >-                  pointer _M_first, _M_last;  // Elements to destroy
> >-                  _Tp_alloc_type& _M_alloc;
> >-
> >-                  _GLIBCXX20_CONSTEXPR
> >-                  _Guard_elts(pointer __first, size_type __n,
> >-                              _Tp_alloc_type& __a)
> >-                  : _M_first(__first), _M_last(__first + __n), _M_alloc(__a)
> >-                  { }
> >-
> >-                  _GLIBCXX20_CONSTEXPR
> >-                  ~_Guard_elts()
> >-                  { std::_Destroy(_M_first, _M_last, _M_alloc); }
> >-
> >-                private:
> >-                  _Guard_elts(const _Guard_elts&);
> >-                };
> >-                _Guard_elts __guard_elts(__new_start + __size, __n, _M_impl);
> >-
> >-                std::__uninitialized_move_if_noexcept_a(
> >-                  __old_start, __old_finish, __new_start,
> >-                  _M_get_Tp_allocator());
> >-
> >-                __guard_elts._M_first = __old_start;
> >-                __guard_elts._M_last = __old_finish;
> >-              }
> >-            _GLIBCXX_ASAN_ANNOTATE_REINIT;
> >-            __guard._M_storage = __old_start;
> >-            __guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
> >+                  _S_relocate(__old_start, __old_finish,
> >+                              __new_start, _M_get_Tp_allocator());
> >+                }
> >+              else
> >+                {
> >+                  // RAII type to destroy initialized elements.
> >+                  struct _Guard_elts
> >+                  {
> >+                    pointer _M_first, _M_last;  // Elements to destroy
> >+                    _Tp_alloc_type& _M_alloc;
> >+
> >+                    _GLIBCXX20_CONSTEXPR
> >+                    _Guard_elts(pointer __first, size_type __n,
> >+                                _Tp_alloc_type& __a)
> >+                    : _M_first(__first), _M_last(__first + __n), _M_alloc(__a)
> >+                    { }
> >+
> >+                    _GLIBCXX20_CONSTEXPR
> >+                    ~_Guard_elts()
> >+                    { std::_Destroy(_M_first, _M_last, _M_alloc); }
> >+
> >+                  private:
> >+                    _Guard_elts(const _Guard_elts&);
> >+                  };
> >+                  _Guard_elts __guard_elts(__new_start + __size, __n, _M_impl);
> >+
> >+                  std::__uninitialized_move_if_noexcept_a(
> >+                    __old_start, __old_finish, __new_start,
> >+                    _M_get_Tp_allocator());
> >+
> >+                  __guard_elts._M_first = __old_start;
> >+                  __guard_elts._M_last = __old_finish;
> >+                }
> >+              _GLIBCXX_ASAN_ANNOTATE_REINIT;
> >+              __guard._M_storage = __old_start;
> >+              __guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
> >+            }
> >+            // deallocate should be called before assignments to _M_impl,
> >+            // to avoid call-clobbering
> >
> >             this->_M_impl._M_start = __new_start;
> >             this->_M_impl._M_finish = __new_start + __size + __n;
> >diff --git a/libstdc++-v3/testsuite/23_containers/vector/110879.cc b/libstdc++-v3/testsuite/23_containers/vector/110879.cc
> >new file mode 100644
> >index 00000000000..d38a5a0d1a3
> >--- /dev/null
> >+++ b/libstdc++-v3/testsuite/23_containers/vector/110879.cc
> >@@ -0,0 +1,35 @@
> >+// -*- C++ -*-
> >+
> >+// Copyright (C) 2023 Free Software Foundation, Inc.
>
> Do you actually have a FSF copyright assignment for GCC?
>
> If not, then this should not be here, as you can't assign it to the
> FSF.
>
> You've already added a Signed-off-by tag, which I assume means you're
> contributing under the terms of the DCO: https://gcc.gnu.org/dco.html
>
> In which case, this isn't copyright FSF.
>
> I've stopped putting the copyright and license header in tests, I
> don't consider a 5 line function that does nothing interesting to be
> copyrightable, or worth adding all this boilerplate to.
>
> If you don't mind, I'll just remove this boilerplate from the test.

I've just copypasted this boilerplate from other test without thinking.
You are correct, I don't have a FSF copyright assignment. I'm contributing
under the terms of the DCO.
You can just remove this boilerplate.

>
>
> >+// 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/>.
> >+
> >+// { dg-do compile }
> >+// { dg-options "-O3 -fdump-tree-optimized" }
> >+
> >+#include <vector>
> >+
> >+std::vector<int> f(std::size_t n) {
> >+  std::vector<int> res;
> >+  for (std::size_t i = 0; i < n; ++i) {
> >+    res.push_back(i);
> >+  }
> >+  return res;
> >+}
> >+
> >+// Reads of _M_finish should be optimized out.
> >+// This regex matches all reads from res variable except for _M_end_of_storage field.
> >+// { dg-final { scan-tree-dump-not "=\\s*\\S*res_(?!\\S*_M_end_of_storage;)" "optimized" } }
>
> If this was added to gcc/testsuite/g++.dg/ instead of
> libstdc++-v3/testsuite then we wouldn't need scandump.exp and
> scantree.exp in the library tests.

Sure, you can move this test to a more appropriate place.

>
> >diff --git a/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp b/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
> >index e8c6504a7f7..1d0588aeadc 100644
> >--- a/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
> >+++ b/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
> >@@ -18,6 +18,19 @@
> >
> > # libstdc++-v3 testsuite that uses the 'dg.exp' driver.
> >
> >+# Damn dejagnu for not having proper library search paths for load_lib.
> >+# We have to explicitly load everything that gcc-dg.exp wants to load.
> >+
> >+proc load_gcc_lib { filename } {
> >+    global srcdir loaded_libs
> >+
> >+    load_file $srcdir/../../gcc/testsuite/lib/$filename
> >+    set loaded_libs($filename) ""
> >+}
> >+
> >+load_gcc_lib scandump.exp
> >+load_gcc_lib scantree.exp
> >+
> > # Initialization.
> > dg-init
> >
>

Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]

[Jonathan Wakely]

On Thu, 17 Aug 2023 at 08:43, Vladimir Palevich <palevichva@gmail.com> wrote:
>
> On Thu, 17 Aug 2023 at 01:51, Jonathan Wakely <jwakely@redhat.com> wrote:
> >
> > On 09/08/23 01:34 +0300, Vladimir Palevich wrote:
> > >Because of the recent change in _M_realloc_insert and _M_default_append, call
> > >to deallocate was ordered after assignment to class members of std::vector
> > >(in the guard destructor), which is causing said members to be call-clobbered.
> > >This is preventing further optimization, the compiler is unable to move memory
> > >read out of a hot loop in this case.
> > >This patch reorders the call to before assignments by putting guard in its own
> > >block. Plus a new testsuite for this case.
> > >I'm not very happy with the new testsuite, but I don't know how to properly
> > >test this.
> > >
> > >Tested on x86_64-pc-linux-gnu.
> > >
> > >Maybe something could be done so that the compiler would be able to optimize
> > >such cases anyway. Reads could be moved just after the clobbering calls in
> > >unlikely branches, for example. This should be a fairly common case with
> > >destructors at the end of a function.
> > >
> > >Note: I don't have write access.
> > >
> > >-- >8 --
> > >
> > >Fix ordering to prevent clobbering of class members by a call to deallocate
> > >in _M_realloc_insert and _M_default_append.
> > >
> > >libstdc++-v3/ChangeLog:
> > >    PR libstdc++/110879
> > >    * include/bits/vector.tcc: End guard lifetime just before assignment to
> > >    class members.
> > >    * testsuite/libstdc++-dg/conformance.exp: Load scantree.exp.
> > >    * testsuite/23_containers/vector/110879.cc: New test.
> > >
> > >Signed-off-by: Vladimir Palevich  <palevichva@gmail.com>
> > >---
> > > libstdc++-v3/include/bits/vector.tcc          | 220 +++++++++---------
> > > .../testsuite/23_containers/vector/110879.cc  |  35 +++
> > > .../testsuite/libstdc++-dg/conformance.exp    |  13 ++
> > > 3 files changed, 163 insertions(+), 105 deletions(-)
> > > create mode 100644 libstdc++-v3/testsuite/23_containers/vector/110879.cc
> > >
> > >diff --git a/libstdc++-v3/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
> > >index ada396c9b30..80631d1e2a1 100644
> > >--- a/libstdc++-v3/include/bits/vector.tcc
> > >+++ b/libstdc++-v3/include/bits/vector.tcc
> > >@@ -488,78 +488,83 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> > >       private:
> > >       _Guard(const _Guard&);
> > >       };
> > >-      _Guard __guard(__new_start, __len, _M_impl);
> > >
> > >-      // The order of the three operations is dictated by the C++11
> > >-      // case, where the moves could alter a new element belonging
> > >-      // to the existing vector.  This is an issue only for callers
> > >-      // taking the element by lvalue ref (see last bullet of C++11
> > >-      // [res.on.arguments]).
> > >+      {
> > >+      _Guard __guard(__new_start, __len, _M_impl);
> > >
> > >-      // If this throws, the existing elements are unchanged.
> > >+      // The order of the three operations is dictated by the C++11
> > >+      // case, where the moves could alter a new element belonging
> > >+      // to the existing vector.  This is an issue only for callers
> > >+      // taking the element by lvalue ref (see last bullet of C++11
> > >+      // [res.on.arguments]).
> > >+
> > >+      // If this throws, the existing elements are unchanged.
> > > #if __cplusplus >= 201103L
> > >-      _Alloc_traits::construct(this->_M_impl,
> > >-                             std::__to_address(__new_start + __elems_before),
> > >-                             std::forward<_Args>(__args)...);
> > >+      _Alloc_traits::construct(this->_M_impl,
> > >+                               std::__to_address(__new_start + __elems_before),
> > >+                               std::forward<_Args>(__args)...);
> > > #else
> > >-      _Alloc_traits::construct(this->_M_impl,
> > >-                             __new_start + __elems_before,
> > >-                             __x);
> > >+      _Alloc_traits::construct(this->_M_impl,
> > >+                               __new_start + __elems_before,
> > >+                               __x);
> > > #endif
> > >
> > > #if __cplusplus >= 201103L
> > >-      if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
> > >-      {
> > >-        // Relocation cannot throw.
> > >-        __new_finish = _S_relocate(__old_start, __position.base(),
> > >-                                   __new_start, _M_get_Tp_allocator());
> > >-        ++__new_finish;
> > >-        __new_finish = _S_relocate(__position.base(), __old_finish,
> > >-                                   __new_finish, _M_get_Tp_allocator());
> > >-      }
> > >-      else
> > >+      if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
> > >+        {
> > >+          // Relocation cannot throw.
> > >+          __new_finish = _S_relocate(__old_start, __position.base(),
> > >+                                     __new_start, _M_get_Tp_allocator());
> > >+          ++__new_finish;
> > >+          __new_finish = _S_relocate(__position.base(), __old_finish,
> > >+                                     __new_finish, _M_get_Tp_allocator());
> > >+        }
> > >+      else
> > > #endif
> > >-      {
> > >-        // RAII type to destroy initialized elements.
> > >-        struct _Guard_elts
> > >         {
> > >-          pointer _M_first, _M_last;  // Elements to destroy
> > >-          _Tp_alloc_type& _M_alloc;
> > >-
> > >-          _GLIBCXX20_CONSTEXPR
> > >-          _Guard_elts(pointer __elt, _Tp_alloc_type& __a)
> > >-          : _M_first(__elt), _M_last(__elt + 1), _M_alloc(__a)
> > >-          { }
> > >-
> > >-          _GLIBCXX20_CONSTEXPR
> > >-          ~_Guard_elts()
> > >-          { std::_Destroy(_M_first, _M_last, _M_alloc); }
> > >-
> > >-        private:
> > >-          _Guard_elts(const _Guard_elts&);
> > >-        };
> > >-
> > >-        // Guard the new element so it will be destroyed if anything throws.
> > >-        _Guard_elts __guard_elts(__new_start + __elems_before, _M_impl);
> > >-
> > >-        __new_finish = std::__uninitialized_move_if_noexcept_a(
> > >-                         __old_start, __position.base(),
> > >-                         __new_start, _M_get_Tp_allocator());
> > >-
> > >-        ++__new_finish;
> > >-        // Guard everything before the new element too.
> > >-        __guard_elts._M_first = __new_start;
> > >-
> > >-        __new_finish = std::__uninitialized_move_if_noexcept_a(
> > >-                          __position.base(), __old_finish,
> > >-                          __new_finish, _M_get_Tp_allocator());
> > >-
> > >-        // New storage has been fully initialized, destroy the old elements.
> > >-        __guard_elts._M_first = __old_start;
> > >-        __guard_elts._M_last = __old_finish;
> > >-      }
> > >-      __guard._M_storage = __old_start;
> > >-      __guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
> > >+          // RAII type to destroy initialized elements.
> > >+          struct _Guard_elts
> > >+          {
> > >+            pointer _M_first, _M_last;  // Elements to destroy
> > >+            _Tp_alloc_type& _M_alloc;
> > >+
> > >+            _GLIBCXX20_CONSTEXPR
> > >+            _Guard_elts(pointer __elt, _Tp_alloc_type& __a)
> > >+            : _M_first(__elt), _M_last(__elt + 1), _M_alloc(__a)
> > >+            { }
> > >+
> > >+            _GLIBCXX20_CONSTEXPR
> > >+            ~_Guard_elts()
> > >+            { std::_Destroy(_M_first, _M_last, _M_alloc); }
> > >+
> > >+          private:
> > >+            _Guard_elts(const _Guard_elts&);
> > >+          };
> > >+
> > >+          // Guard the new element so it will be destroyed if anything throws.
> > >+          _Guard_elts __guard_elts(__new_start + __elems_before, _M_impl);
> > >+
> > >+          __new_finish = std::__uninitialized_move_if_noexcept_a(
> > >+                           __old_start, __position.base(),
> > >+                           __new_start, _M_get_Tp_allocator());
> > >+
> > >+          ++__new_finish;
> > >+          // Guard everything before the new element too.
> > >+          __guard_elts._M_first = __new_start;
> > >+
> > >+          __new_finish = std::__uninitialized_move_if_noexcept_a(
> > >+                            __position.base(), __old_finish,
> > >+                            __new_finish, _M_get_Tp_allocator());
> > >+
> > >+          // New storage has been fully initialized, destroy the old elements.
> > >+          __guard_elts._M_first = __old_start;
> > >+          __guard_elts._M_last = __old_finish;
> > >+        }
> > >+      __guard._M_storage = __old_start;
> > >+      __guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
> > >+      }
> > >+      // deallocate should be called before assignments to _M_impl,
> > >+      // to avoid call-clobbering
> > >
> > >       this->_M_impl._M_start = __new_start;
> > >       this->_M_impl._M_finish = __new_finish;
> > >@@ -728,49 +733,54 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> > >             private:
> > >               _Guard(const _Guard&);
> > >             };
> > >-            _Guard __guard(__new_start, __len, _M_impl);
> > >-
> > >-            std::__uninitialized_default_n_a(__new_start + __size, __n,
> > >-                                             _M_get_Tp_allocator());
> > >-
> > >-            if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
> > >-              {
> > >-                _S_relocate(__old_start, __old_finish,
> > >-                            __new_start, _M_get_Tp_allocator());
> > >-              }
> > >-            else
> > >-              {
> > >-                // RAII type to destroy initialized elements.
> > >-                struct _Guard_elts
> > >+
> > >+            {
> > >+              _Guard __guard(__new_start, __len, _M_impl);
> > >+
> > >+              std::__uninitialized_default_n_a(__new_start + __size, __n,
> > >+                                               _M_get_Tp_allocator());
> > >+
> > >+              if _GLIBCXX17_CONSTEXPR (_S_use_relocate())
> > >                 {
> > >-                  pointer _M_first, _M_last;  // Elements to destroy
> > >-                  _Tp_alloc_type& _M_alloc;
> > >-
> > >-                  _GLIBCXX20_CONSTEXPR
> > >-                  _Guard_elts(pointer __first, size_type __n,
> > >-                              _Tp_alloc_type& __a)
> > >-                  : _M_first(__first), _M_last(__first + __n), _M_alloc(__a)
> > >-                  { }
> > >-
> > >-                  _GLIBCXX20_CONSTEXPR
> > >-                  ~_Guard_elts()
> > >-                  { std::_Destroy(_M_first, _M_last, _M_alloc); }
> > >-
> > >-                private:
> > >-                  _Guard_elts(const _Guard_elts&);
> > >-                };
> > >-                _Guard_elts __guard_elts(__new_start + __size, __n, _M_impl);
> > >-
> > >-                std::__uninitialized_move_if_noexcept_a(
> > >-                  __old_start, __old_finish, __new_start,
> > >-                  _M_get_Tp_allocator());
> > >-
> > >-                __guard_elts._M_first = __old_start;
> > >-                __guard_elts._M_last = __old_finish;
> > >-              }
> > >-            _GLIBCXX_ASAN_ANNOTATE_REINIT;
> > >-            __guard._M_storage = __old_start;
> > >-            __guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
> > >+                  _S_relocate(__old_start, __old_finish,
> > >+                              __new_start, _M_get_Tp_allocator());
> > >+                }
> > >+              else
> > >+                {
> > >+                  // RAII type to destroy initialized elements.
> > >+                  struct _Guard_elts
> > >+                  {
> > >+                    pointer _M_first, _M_last;  // Elements to destroy
> > >+                    _Tp_alloc_type& _M_alloc;
> > >+
> > >+                    _GLIBCXX20_CONSTEXPR
> > >+                    _Guard_elts(pointer __first, size_type __n,
> > >+                                _Tp_alloc_type& __a)
> > >+                    : _M_first(__first), _M_last(__first + __n), _M_alloc(__a)
> > >+                    { }
> > >+
> > >+                    _GLIBCXX20_CONSTEXPR
> > >+                    ~_Guard_elts()
> > >+                    { std::_Destroy(_M_first, _M_last, _M_alloc); }
> > >+
> > >+                  private:
> > >+                    _Guard_elts(const _Guard_elts&);
> > >+                  };
> > >+                  _Guard_elts __guard_elts(__new_start + __size, __n, _M_impl);
> > >+
> > >+                  std::__uninitialized_move_if_noexcept_a(
> > >+                    __old_start, __old_finish, __new_start,
> > >+                    _M_get_Tp_allocator());
> > >+
> > >+                  __guard_elts._M_first = __old_start;
> > >+                  __guard_elts._M_last = __old_finish;
> > >+                }
> > >+              _GLIBCXX_ASAN_ANNOTATE_REINIT;
> > >+              __guard._M_storage = __old_start;
> > >+              __guard._M_len = this->_M_impl._M_end_of_storage - __old_start;
> > >+            }
> > >+            // deallocate should be called before assignments to _M_impl,
> > >+            // to avoid call-clobbering
> > >
> > >             this->_M_impl._M_start = __new_start;
> > >             this->_M_impl._M_finish = __new_start + __size + __n;
> > >diff --git a/libstdc++-v3/testsuite/23_containers/vector/110879.cc b/libstdc++-v3/testsuite/23_containers/vector/110879.cc
> > >new file mode 100644
> > >index 00000000000..d38a5a0d1a3
> > >--- /dev/null
> > >+++ b/libstdc++-v3/testsuite/23_containers/vector/110879.cc
> > >@@ -0,0 +1,35 @@
> > >+// -*- C++ -*-
> > >+
> > >+// Copyright (C) 2023 Free Software Foundation, Inc.
> >
> > Do you actually have a FSF copyright assignment for GCC?
> >
> > If not, then this should not be here, as you can't assign it to the
> > FSF.
> >
> > You've already added a Signed-off-by tag, which I assume means you're
> > contributing under the terms of the DCO: https://gcc.gnu.org/dco.html
> >
> > In which case, this isn't copyright FSF.
> >
> > I've stopped putting the copyright and license header in tests, I
> > don't consider a 5 line function that does nothing interesting to be
> > copyrightable, or worth adding all this boilerplate to.
> >
> > If you don't mind, I'll just remove this boilerplate from the test.
>
> I've just copypasted this boilerplate from other test without thinking.
> You are correct, I don't have a FSF copyright assignment. I'm contributing
> under the terms of the DCO.
> You can just remove this boilerplate.
>
> >
> >
> > >+// 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/>.
> > >+
> > >+// { dg-do compile }
> > >+// { dg-options "-O3 -fdump-tree-optimized" }
> > >+
> > >+#include <vector>
> > >+
> > >+std::vector<int> f(std::size_t n) {
> > >+  std::vector<int> res;
> > >+  for (std::size_t i = 0; i < n; ++i) {
> > >+    res.push_back(i);
> > >+  }
> > >+  return res;
> > >+}
> > >+
> > >+// Reads of _M_finish should be optimized out.
> > >+// This regex matches all reads from res variable except for _M_end_of_storage field.
> > >+// { dg-final { scan-tree-dump-not "=\\s*\\S*res_(?!\\S*_M_end_of_storage;)" "optimized" } }
> >
> > If this was added to gcc/testsuite/g++.dg/ instead of
> > libstdc++-v3/testsuite then we wouldn't need scandump.exp and
> > scantree.exp in the library tests.
>
> Sure, you can move this test to a more appropriate place.

OK thanks - I've pushed it to trunk now.

Thanks again for the analysis of the problem and the patch to fix it!


>
> >
> > >diff --git a/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp b/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
> > >index e8c6504a7f7..1d0588aeadc 100644
> > >--- a/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
> > >+++ b/libstdc++-v3/testsuite/libstdc++-dg/conformance.exp
> > >@@ -18,6 +18,19 @@
> > >
> > > # libstdc++-v3 testsuite that uses the 'dg.exp' driver.
> > >
> > >+# Damn dejagnu for not having proper library search paths for load_lib.
> > >+# We have to explicitly load everything that gcc-dg.exp wants to load.
> > >+
> > >+proc load_gcc_lib { filename } {
> > >+    global srcdir loaded_libs
> > >+
> > >+    load_file $srcdir/../../gcc/testsuite/lib/$filename
> > >+    set loaded_libs($filename) ""
> > >+}
> > >+
> > >+load_gcc_lib scandump.exp
> > >+load_gcc_lib scantree.exp
> > >+
> > > # Initialization.
> > > dg-init
> > >
> >
>

Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]

[Jonathan Wakely]

At Marek and Jason's suggestion I've moved the new test to a subdir:

   c++: Move new test to 'opt' sub-directory

   gcc/testsuite/ChangeLog:

           * g++.dg/pr110879.C: Moved to...
           * g++.dg/opt/pr110879.C: ...here.