* [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]
@ 2023-08-08 22:34 Vladimir Palevich
2023-08-16 17:41 ` Jonathan Wakely
2023-08-16 22:51 ` Jonathan Wakely
0 siblings, 2 replies; 6+ messages in thread
From: Vladimir Palevich @ 2023-08-08 22:34 UTC (permalink / raw)
To: gcc-patches; +Cc: Vladimir Palevich
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.
+//
+// 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
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]
2023-08-08 22:34 [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] Vladimir Palevich
@ 2023-08-16 17:41 ` Jonathan Wakely
2023-08-16 22:51 ` Jonathan Wakely
1 sibling, 0 replies; 6+ 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] 6+ messages in thread
* Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]
2023-08-08 22:34 [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] Vladimir Palevich
2023-08-16 17:41 ` Jonathan Wakely
@ 2023-08-16 22:51 ` Jonathan Wakely
2023-08-17 7:43 ` Vladimir Palevich
1 sibling, 1 reply; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ messages in thread
end of thread, other threads:[~2023-09-01 16:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-08 22:34 [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879] Vladimir Palevich
2023-08-16 17:41 ` 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).