From: Jonathan Wakely <jwakely@redhat.com>
To: Vladimir Palevich <palevichva@gmail.com>
Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org
Subject: Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]
Date: Wed, 16 Aug 2023 23:51:54 +0100 [thread overview]
Message-ID: <ZN1TCkoopGhpS4Gy@redhat.com> (raw)
In-Reply-To: <20230808223405.35178-1-palevichva@gmail.com>
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
>
next prev parent reply other threads:[~2023-08-16 22:51 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20230808223405.35178-1-palevichva@gmail.com>
2023-08-16 17:41 ` Jonathan Wakely
2023-08-16 22:51 ` Jonathan Wakely [this message]
2023-08-17 7:43 ` Vladimir Palevich
2023-09-01 15:12 ` Jonathan Wakely
2023-09-01 16:55 ` Jonathan Wakely
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=ZN1TCkoopGhpS4Gy@redhat.com \
--to=jwakely@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=libstdc++@gcc.gnu.org \
--cc=palevichva@gmail.com \
/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: link
Be 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).