public inbox for gcc-cvs@sourceware.org help / color / mirror / Atom feed
From: Jonathan Wakely <redi@gcc.gnu.org> To: gcc-cvs@gcc.gnu.org, libstdc++-cvs@gcc.gnu.org Subject: [gcc r14-3627] libstdc++: fix memory clobbering in std::vector [PR110879] Date: Fri, 1 Sep 2023 15:02:29 +0000 (GMT) [thread overview] Message-ID: <20230901150229.BA5023857714@sourceware.org> (raw) https://gcc.gnu.org/g:419c423d3aeca754e47e1ce1bf707735603a90a3 commit r14-3627-g419c423d3aeca754e47e1ce1bf707735603a90a3 Author: Vladimir Palevich <palevichva@gmail.com> 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 <palevichva@gmail.com> 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 <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/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;
reply other threads:[~2023-09-01 15:02 UTC|newest] Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20230901150229.BA5023857714@sourceware.org \ --to=redi@gcc.gnu.org \ --cc=gcc-cvs@gcc.gnu.org \ --cc=libstdc++-cvs@gcc.gnu.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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).