Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]
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.
Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]
On Thu, 17 Aug 2023 at 08:43, Vladimir Palevich wrote:
>
> On Thu, 17 Aug 2023 at 01:51, Jonathan Wakely 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
> > >---
> > > 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;
> > >+
Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]
On Thu, 17 Aug 2023 at 01:51, Jonathan Wakely 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
> >---
> > 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
> > {
Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]
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
---
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,
Re: [PATCH] libstdc++: fix memory clobbering in std::vector [PR110879]
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
---
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
