On Thu, 17 Aug 2023 at 08:43, Vladimir Palevich <palevic...@gmail.com> wrote:
>
> On Thu, 17 Aug 2023 at 01:51, Jonathan Wakely <jwak...@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 <palevic...@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
> > >
> >
>
