libstdc++: fix memory clobbering in std::vector [PR110879]

Fix ordering to prevent clobbering of class members by a call to deallocate
in _M_realloc_insert and _M_default_append.

Because of recent changes in _M_realloc_insert and _M_default_append,
calls to deallocate were 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.

	PR libstdc++/110879

libstdc++-v3/ChangeLog:

	* include/bits/vector.tcc (_M_realloc_insert): End guard
	lifetime just before assignment to class members.
	(_M_default_append): Likewise.

gcc/testsuite/ChangeLog:

	* g++.dg/pr110879.C: New test.

Signed-off-by: Vladimir Palevich  <palevichva@gmail.com>

diff --git a/gcc/testsuite/g++.dg/pr110879.C b/gcc/testsuite/g++.dg/pr110879.C
new file mode 100644 (file)
index 0000000..7f0a0a8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr110879.C
@@ -0,0 +1,16 @@
+// { 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/include/bits/vector.tcc b/libstdc++-v3/include/bits/vector.tcc
index ada396c9b30aa6c548d57a393d63f1209370e109..80631d1e2a19d3f1eb9f2450b68e63c085b6c19c 100644 (file)
--- 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);
+
+       // 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 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;
+           // 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(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); }
+             _GLIBCXX20_CONSTEXPR
+             ~_Guard_elts()
+             { std::_Destroy(_M_first, _M_last, _M_alloc); }
 
-         private:
-           _Guard_elts(const _Guard_elts&);
-         };
+           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);
+           // 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 = 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;
+           // 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_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;
+           // 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());
+             {
+               _Guard __guard(__new_start, __len, _M_impl);
 
-             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
+               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;