* Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] [not found] <20230808223405.35178-1-palevichva@gmail.com> @ 2023-08-16 17:41 ` Jonathan Wakely 2023-08-16 22:51 ` Jonathan Wakely 1 sibling, 0 replies; 5+ messages in thread From: Jonathan Wakely @ 2023-08-16 17:41 UTC (permalink / raw) To: Vladimir Palevich; +Cc: gcc-patches, libstdc++ 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] [not found] <20230808223405.35178-1-palevichva@gmail.com> 2023-08-16 17:41 ` [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] Jonathan Wakely @ 2023-08-16 22:51 ` Jonathan Wakely 2023-08-17 7:43 ` Vladimir Palevich 1 sibling, 1 reply; 5+ messages in thread From: Jonathan Wakely @ 2023-08-16 22:51 UTC (permalink / raw) To: Vladimir Palevich; +Cc: gcc-patches, libstdc++ 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] 2023-08-16 22:51 ` Jonathan Wakely @ 2023-08-17 7:43 ` Vladimir Palevich 2023-09-01 15:12 ` Jonathan Wakely 0 siblings, 1 reply; 5+ messages in thread From: Vladimir Palevich @ 2023-08-17 7:43 UTC (permalink / raw) To: Jonathan Wakely; +Cc: gcc-patches, libstdc++ 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 > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] 2023-08-17 7:43 ` Vladimir Palevich @ 2023-09-01 15:12 ` Jonathan Wakely 2023-09-01 16:55 ` Jonathan Wakely 0 siblings, 1 reply; 5+ messages in thread From: Jonathan Wakely @ 2023-09-01 15:12 UTC (permalink / raw) To: Vladimir Palevich; +Cc: gcc-patches, libstdc++ 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 > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] 2023-09-01 15:12 ` Jonathan Wakely @ 2023-09-01 16:55 ` Jonathan Wakely 0 siblings, 0 replies; 5+ messages in thread From: Jonathan Wakely @ 2023-09-01 16:55 UTC (permalink / raw) To: Vladimir Palevich; +Cc: gcc-patches, libstdc++ 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-09-01 16:55 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20230808223405.35178-1-palevichva@gmail.com> 2023-08-16 17:41 ` [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] Jonathan Wakely 2023-08-16 22:51 ` Jonathan Wakely 2023-08-17 7:43 ` Vladimir Palevich 2023-09-01 15:12 ` Jonathan Wakely 2023-09-01 16:55 ` Jonathan Wakely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).