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  <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.
+//
+// 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


Reply via email to