From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2181) id BA5023857714; Fri, 1 Sep 2023 15:02:29 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org BA5023857714 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1693580549; bh=jcSIv/IFl3/NGNYa/1yebbZaYreIL2EBVOyqgj1AtXI=; h=From:To:Subject:Date:From; b=CbWVokk9WrSRqm8UM0XvPdXkFf4YwTwpdx4JcSwSTa3dr54FruUMLoOEwbxtsTh4X PsHQSGPKuFOLVEFsyfO6a3AabdfGMDSfPC/G17yuz/4Bd7lWwhFCF5XCrGQEObBflU WJYINhkp/1n8RODRr7dkNlQOnwtEtVYK2TpijXMM= MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: Jonathan Wakely To: gcc-cvs@gcc.gnu.org, libstdc++-cvs@gcc.gnu.org Subject: [gcc r14-3627] libstdc++: fix memory clobbering in std::vector [PR110879] X-Act-Checkin: gcc X-Git-Author: Vladimir Palevich X-Git-Refname: refs/heads/master X-Git-Oldrev: 283994cba687e0746f9a1018bae2c41291e362ff X-Git-Newrev: 419c423d3aeca754e47e1ce1bf707735603a90a3 Message-Id: <20230901150229.BA5023857714@sourceware.org> Date: Fri, 1 Sep 2023 15:02:29 +0000 (GMT) List-Id: https://gcc.gnu.org/g:419c423d3aeca754e47e1ce1bf707735603a90a3 commit r14-3627-g419c423d3aeca754e47e1ce1bf707735603a90a3 Author: Vladimir Palevich Date: Wed Aug 9 01:34:05 2023 +0300 libstdc++: fix memory clobbering in std::vector [PR110879] Fix ordering to prevent clobbering of class members by a call to deallocate in _M_realloc_insert and _M_default_append. Because of recent changes in _M_realloc_insert and _M_default_append, calls to deallocate were 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. PR libstdc++/110879 libstdc++-v3/ChangeLog: * include/bits/vector.tcc (_M_realloc_insert): End guard lifetime just before assignment to class members. (_M_default_append): Likewise. gcc/testsuite/ChangeLog: * g++.dg/pr110879.C: New test. Signed-off-by: Vladimir Palevich Diff: --- gcc/testsuite/g++.dg/pr110879.C | 16 +++ libstdc++-v3/include/bits/vector.tcc | 200 ++++++++++++++++++----------------- 2 files changed, 121 insertions(+), 95 deletions(-) diff --git a/gcc/testsuite/g++.dg/pr110879.C b/gcc/testsuite/g++.dg/pr110879.C new file mode 100644 index 000000000000..7f0a0a80b8af --- /dev/null +++ b/gcc/testsuite/g++.dg/pr110879.C @@ -0,0 +1,16 @@ +// { dg-do compile } +// { dg-options "-O3 -fdump-tree-optimized" } + +#include + +std::vector f(std::size_t n) { + std::vector 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/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc index ada396c9b30a..80631d1e2a19 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); + + // 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 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; + // 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(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); } + _GLIBCXX20_CONSTEXPR + ~_Guard_elts() + { std::_Destroy(_M_first, _M_last, _M_alloc); } - private: - _Guard_elts(const _Guard_elts&); - }; + 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); + // 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 = 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; + // 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_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; + // 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()); + { + _Guard __guard(__new_start, __len, _M_impl); - 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 + 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;